Skip to content

ENT-1544 B3: OpenCV/FFmpeg RTSPS TLS fallback - #2727

Merged
PawelPeczek-Roboflow merged 6 commits into
mainfrom
nv/ent-1544/b3-inference-opencv-rtsps
Jul 31, 2026
Merged

ENT-1544 B3: OpenCV/FFmpeg RTSPS TLS fallback#2727
PawelPeczek-Roboflow merged 6 commits into
mainfrom
nv/ent-1544/b3-inference-opencv-rtsps

Conversation

@NVergunst-ROBO

@NVergunst-ROBO NVergunst-ROBO commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • OpenCV/FFmpeg RTSPS fallback via OPENCV_FFMPEG_CAPTURE_OPTIONS when GStreamer path unavailable.
  • Shared rtsp_tls helpers for GStreamer and OpenCV ingest paths.

ENT-1544

Phase B3 — non-Jetson / fallback RTSPS. Plan: cursor-cloud/features/ent-1544/plans/B3-inference-opencv.md.

Replaces closed #2724 after branch rename.

Dependencies

  • B2 GStreamer path preferred; B1 for CA env.

How has this change been tested?

  • pytest tests/inference/unit_tests/core/interfaces/camera/test_rtsp_opencv_tls.py — non-RTSPS, CA bundle (GST_SSL_CA_CERTIFICATE / SSL_CERT_FILE), self-signed flags (0 and 126), strict verify unchanged, gstreamer suffix helper, env apply + previous-value restore.

Test plan

  • Unit tests for FFmpeg option builder
  • Wire into video_source / OpenCV capture open path (follow-up)

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

👋 Thanks for the pull request! Here is how automated Claude review works here, so you spend credits (and reviewer time) wisely.

🚦 This PR is marked Ready for review, so automated Claude review will run — and every pass spends real credits.

Warning

💸 The Claude reviewer bills in credits, not vibes

Automated review spins up a real agent that reads real code and spends real credits on every pass. It is glad to help — but it is not a rubber duck, a linter you poke in a loop, or a substitute for reading the contributing guide. Treat it like an expensive senior reviewer whose time you booked, and show up prepared.

Draft when unsure, Ready when you mean it:

  • 🌱 Not sure the PR is in good shape yet? Keep it (or set it back) as a draft — drafts pause review, so you can push and iterate without burning credits on a moving target.
  • 💪 Feel strong about the contents? Mark it Ready for review and the reviewer will take a look.

However you get there, arrive prepared:

  • 🧱 Bring a SOLID, thorough PR. Point your local agent at our skills/ to tune it to our guidelines first — or, if you are one of those fabled carbon-based contributors, read them yourself. A half-baked diff costs exactly the same to review as a finished one.
  • Resolve every comment before you re-request review. Re-requesting with threads still open means paying twice for the same conversation.
  • 🔁 Do not use CI review as an inner loop for a local agent. The reviewer is not a step-by-step debugger — do the unfolding locally and arrive with the answer, not the search.
  • 🙋 If something looks off, ask a human. One question to a maintainer is cheaper and faster than three rounds of agent re-review chasing a misread.

Reviews are not free. A draft costs nothing to review; a Ready PR is a promise that it is worth reviewing.

  • Prefer to skip automated review entirely? Add the skip-claude-review label.

@CLAassistant

CLAassistant commented Jul 28, 2026

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ NVergunst-ROBO
✅ grzegorz-roboflow
❌ Nicholas Vergunst


Nicholas Vergunst seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions

Copy link
Copy Markdown
Contributor

🤖 Claude review started at commit 9740eda881129c8a4c0a661aeeba455b97e1ab45.

New commits are not auto-reviewed. Add the claude-review label (remove & re-add it to trigger again) when you want another review.

Comment thread inference/core/interfaces/camera/rtsp_opencv_tls.py Outdated
Comment thread inference/core/interfaces/camera/rtsp_opencv_tls.py Outdated
@github-actions

Copy link
Copy Markdown
Contributor

@NVergunst-ROBO — thanks for the PR. A few things need clarifying before this can advance.

This PR is ON HOLD pending your answers. The review will not proceed to sign-off, and the PR should not be merged, until the IMPORTANT questions below are addressed.

No hard blocking bug was found in the isolated builder logic — but that's largely because these helpers are dead code: a repo-wide search shows build_opencv_ffmpeg_capture_options, apply_opencv_rtsps_tls_env, and rtsp_tls_validation_flags_gstreamer_suffix are imported/called only by each other and the new test. Nothing in the ingest path (video_source.py, camera.py) uses them. That makes the important concerns integration-level, so I'm raising them as questions rather than findings.

Questions

  1. IMPORTANT — integration intent. Is this deliberately an inert scaffold (B-series), with wiring into the OpenCV/FFmpeg VideoCapture open path to come in a later PR? If so, where/when does that wiring land? This matters because the security and concurrency concerns below only become live once wired, and their answers determine whether the design is safe.

  2. IMPORTANT — process-global env mutation / per-stream isolation. apply_opencv_rtsps_tls_env writes the process-global OPENCV_FFMPEG_CAPTURE_OPTIONS. Once wired, multiple concurrent RTSP(S) sources in one process (different CA bundles, or one self-signed with tls_verify;0 alongside one that must verify) would clobber each other — last writer wins at capture-open time. The returned "previous value" has no restore call site in this PR. How is per-stream isolation intended to work? (See inline comment on line 41.)

  3. IMPORTANT — TLS verification disable + env-var documentation. ROBOFLOW_RTSP_TLS_VALIDATION_FLAGS=0 disables certificate verification (tls_verify;0), a MITM-exposing knob. Please confirm this is intended, and whether the three new env vars (ROBOFLOW_RTSP_TLS_VALIDATION_FLAGS, GST_SSL_CA_CERTIFICATE, SSL_CERT_FILE) should be surfaced in inference/core/env.py / user docs. Right now none of them are documented anywhere. (See inline comment on line 30.)

  4. optional — FFmpeg option key. Is cafile the correct/accepted option key for your target FFmpeg build (some versions expose it as ca_file)? Nothing exercises this against a real FFmpeg, so it's unverified.

  5. optional — naming. The PR title says B3 but the commit message and both module docstrings say B2 (and rtsp_tls.py says "B1/B2"). Please align so the phase is unambiguous.

Note on tests

The unit tests cover the builder happy paths and are isolated (via monkeypatch). Consider adding: tls_verify absent when the flag is unset, a non-"0" flag value is ignored, and apply_* returns the prior env value.

Re-review

New commits are NOT auto-reviewed. Once you've answered and/or pushed changes, add the claude-review label (remove and re-add it to re-trigger) to request a fresh review. Unanswered IMPORTANT questions may keep this out of a release.

Reviewed at HEAD: 9740eda

@NVergunst-ROBO
NVergunst-ROBO force-pushed the nv/ent-1544/b3-inference-opencv-rtsps branch from 69fd896 to d26d695 Compare July 31, 2026 16:20
@NVergunst-ROBO

Copy link
Copy Markdown
Contributor Author

Same CLA cleanup as #2725 — rewrote branch onto current main as a single commit with my author only (removed the old Cursor co-authored commits and bad author email).

Expanded unit tests for CA fallback, strict-verify unchanged, gstreamer suffix helper, and apply_opencv_rtsps_tls_env previous-value return. Documented in code that OPENCV_FFMPEG_CAPTURE_OPTIONS is process-global and callers must restore when wiring multi-source paths.

This remains an inert scaffold — helpers are not imported by video_source yet; wiring is a follow-up commit.

@grzegorz-roboflow

grzegorz-roboflow commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator
## Findings

  - P1 — Strict RTSPS verification is never enabled.
    inference/core/interfaces/camera/rtsp_opencv_tls.py:28 only emits tls_verify;0;
    unset or “strict” flags emit nothing. FFmpeg defaults tls_verify to 0, so even
    a configured CA file is unused and untrusted certificates are accepted. Emit
    tls_verify;1 for the default/strict path and invert
    tests/inference/unit_tests/core/interfaces/camera/test_rtsp_opencv_tls.py:53.
    FFmpeg TLS defaults (https://www.ffmpeg.org/doxygen/8.0/tls_8h.html)

  - P1 — The process-global apply/restore API is racy. inference/core/interfaces/
    camera/rtsp_opencv_tls.py:39 mutates an environment variable before OpenCV
    acquires its internal open lock. Concurrent source initialization can read
    another source’s options and restore values out of order. Use one shared lock/
    context manager spanning set → VideoCapture open → restore.

  - P2 — Existing OpenCV capture options are discarded.
    inference/core/interfaces/camera/rtsp_opencv_tls.py:49 replaces options such as
    timeouts, buffer size, or hardware acceleration. Returning the old value
    afterward does not help the capture being opened. Merge unrelated existing keys
    while overriding only transport/TLS keys. OpenCV FFmpeg backend

    (https://github.com/opencv/opencv/blob/4.10.0/modules/videoio/src/cap_ffmpeg_impl.hpp)

  - P2 — B3 does not compose with the preceding B1/B2 branches. inference/core/
    interfaces/camera/rtsp_tls.py:12 recreates B1’s shared module but omits
    is_rtsp_url, which B2 imports. This produces an add/add conflict; choosing B3’s
    version breaks B2 at import time. Rebase B3 onto B2/B1 and add only the OpenCV-
    specific constant/helper.
    ```

Nicholas Vergunst and others added 4 commits July 31, 2026 13:33
Introduces rtsp_tls module with ROBOFLOW_RTSP_TLS_VALIDATION_FLAGS parsing
ported from agent/fix-jetson-nvjpeg-runtime. Full Jetson rtspsrc wiring in follow-up.

Co-authored-by: Cursor <cursoragent@cursor.com>
Wire rtsps:// on Jetson to a GStreamer CAP_GSTREAMER pipeline with
user-id/user-pw auth (not creds in location), ROBOFLOW_RTSP_TLS_VALIDATION_FLAGS,
and GST_SSL_CA_CERTIFICATE propagation. Plain rtsp:// stays on CV2.

Co-authored-by: Cursor <cursoragent@cursor.com>
parsebin/avdec_h264 hung or were missing on nick-shed; rtph264depay through
nvv4l2decoder is the working E2E decode path on Jetson inference images.

Co-authored-by: Cursor <cursoragent@cursor.com>
…nv lock.

Emit tls_verify;1 for default/strict RTSPS paths, merge existing
OPENCV_FFMPEG_CAPTURE_OPTIONS keys, and expose opencv_rtsps_tls_env context
manager with a process lock spanning set → VideoCapture open → restore.
@NVergunst-ROBO
NVergunst-ROBO force-pushed the nv/ent-1544/b3-inference-opencv-rtsps branch from d26d695 to 209808a Compare July 31, 2026 17:34
@NVergunst-ROBO

Copy link
Copy Markdown
Contributor Author

Addressed all four items in 209808a:

P1 — strict verify: default/unset and non-relaxed flag values now emit tls_verify;1 (FFmpeg defaults verify off). tls_verify;0 only for flags 0 and 126.

P1 — concurrency: replaced apply_opencv_rtsps_tls_env with opencv_rtsps_tls_env context manager. A process-wide lock spans env set → VideoCapture open → restore so concurrent RTSPS sources cannot clobber each other.

P2 — merge existing options: merge_opencv_ffmpeg_capture_options parses existing OPENCV_FFMPEG_CAPTURE_OPTIONS and overrides only transport/TLS keys (rtsp_transport, cafile, tls_verify).

P2 — B2 composition: rebased onto nv/ent-1544/b2-inference-gstreamer-rtsps then origin/main. B3 adds only rtsp_opencv_tls.py + tests; rtsp_tls.py comes from B2 unchanged (is_rtsp_url intact).

@NVergunst-ROBO
NVergunst-ROBO enabled auto-merge (squash) July 31, 2026 17:41
@PawelPeczek-Roboflow
PawelPeczek-Roboflow merged commit a34f929 into main Jul 31, 2026
35 of 38 checks passed
@PawelPeczek-Roboflow
PawelPeczek-Roboflow deleted the nv/ent-1544/b3-inference-opencv-rtsps branch July 31, 2026 17:45
NVergunst-ROBO added a commit that referenced this pull request Jul 31, 2026
B2 GStreamer RTSPS wiring landed on main via B3 (#2727). This commit
rebases the branch and adds the negative-integer validation test from
Claude review feedback.
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