Skip to content

ENT-1686/1687: Harden OpenCV RTSPS capture opens - #2792

Open
NVergunst-ROBO wants to merge 6 commits into
mainfrom
nv/ent-1686-1687-opencv-rtsps-hardening
Open

ENT-1686/1687: Harden OpenCV RTSPS capture opens#2792
NVergunst-ROBO wants to merge 6 commits into
mainfrom
nv/ent-1686-1687-opencv-rtsps-hardening

Conversation

@NVergunst-ROBO

@NVergunst-ROBO NVergunst-ROBO commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Description

Hardens the OpenCV/FFmpeg RTSPS path wired in inference#2776 (ENT-1677).

Changes:

  1. ENT-1686: Default FFmpeg stimeout (5s) on RTSPS opens so one unreachable camera cannot hold the global TLS env lock for minutes.
  2. ENT-1687: Pin cv2.CAP_FFMPEG for rtsps:// URLs so OPENCV_FFMPEG_CAPTURE_OPTIONS cannot be silently ignored by another backend. Log a warning when tls_validation_flags=126 (allow_self_signed) maps to full tls_verify=0 on FFmpeg (weaker than GStreamer's partial mode — documented, permanent product choice).

Depends on: inference#2776 — merge that first, then this.

Linear: ENT-1686, ENT-1687

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Maintenance (non-breaking, non-user facing changing such as dependency update, adding test, modifying secrets, etc.)
  • This change requires a documentation update

How has this change been tested, please provide a testcase or example of how you tested the change?

Will the change affect Universe? If so was this change tested in universe?

No. Edge inference pipeline path only.

Any specific deployment considerations

Ship in the same inference release as #2776 (or immediately after). No config migrations.

Docs

  • Docs updated? What were the changes: N/A

@CLAassistant

CLAassistant commented Aug 13, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@NVergunst-ROBO
NVergunst-ROBO force-pushed the nv/bug/ent-1677-opencv-rtsps-tls-verify branch from b3d41ea to 214abfb Compare August 17, 2026 19:08
@NVergunst-ROBO
NVergunst-ROBO force-pushed the nv/ent-1686-1687-opencv-rtsps-hardening branch from a5db576 to fb18db6 Compare August 18, 2026 01:31
@NVergunst-ROBO
NVergunst-ROBO force-pushed the nv/bug/ent-1677-opencv-rtsps-tls-verify branch from 6c46903 to a96eaf1 Compare August 18, 2026 02:30
@NVergunst-ROBO
NVergunst-ROBO force-pushed the nv/ent-1686-1687-opencv-rtsps-hardening branch from fb18db6 to f1ff49c Compare August 18, 2026 02:30
@NVergunst-ROBO
NVergunst-ROBO marked this pull request as ready for review August 19, 2026 18:23
@NVergunst-ROBO
NVergunst-ROBO force-pushed the nv/bug/ent-1677-opencv-rtsps-tls-verify branch from a96eaf1 to 5d9ab74 Compare August 19, 2026 18:26
@NVergunst-ROBO
NVergunst-ROBO force-pushed the nv/ent-1686-1687-opencv-rtsps-hardening branch from f1ff49c to cd32b96 Compare August 19, 2026 18:26
@NVergunst-ROBO
NVergunst-ROBO changed the base branch from nv/bug/ent-1677-opencv-rtsps-tls-verify to main August 19, 2026 18:26
@dkosowski87 dkosowski87 added the claude-review Use to trigger AI review manually label Aug 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🤖 Claude review started at commit 0e66f1f96488d5fcad28dbd946cb1d32db1121d5.

New commits are not auto-reviewed. Add the claude-review label to request a re-review — the label is consumed when the review starts, so just add it again next time.

@github-actions github-actions Bot removed the claude-review Use to trigger AI review manually label Aug 20, 2026
Comment thread inference/core/interfaces/camera/rtsp_opencv_tls.py Outdated
@NVergunst-ROBO
NVergunst-ROBO force-pushed the nv/ent-1686-1687-opencv-rtsps-hardening branch from 0e66f1f to 62bd953 Compare August 20, 2026 23:37
@NVergunst-ROBO

Copy link
Copy Markdown
Contributor Author

Re: High finding on stimeout alone — fixed in 62bd9530b.

  • Sets both stimeout and timeout when neither is user-supplied (FFmpeg 7.1.3 path).
  • Unit test asserts both keys.
  • Module comment reworded: socket I/O timeout applies for the whole session, not just open.

Branch rebased cleanly on #2776 (c7de9095a); merge #2776 first, then this PR. Requesting re-review with claude-review label.

@NVergunst-ROBO NVergunst-ROBO added the claude-review Use to trigger AI review manually label Aug 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🤖 Claude review started at commit 62bd9530b9130c45f6e5c514395650de778aca04.

New commits are not auto-reviewed. Add the claude-review label to request a re-review — the label is consumed when the review starts, so just add it again next time.

@github-actions github-actions Bot removed the claude-review Use to trigger AI review manually label Aug 20, 2026
Comment thread inference/core/interfaces/camera/rtsp_opencv_tls.py Outdated
Comment thread inference/core/interfaces/camera/rtsp_opencv_tls.py
@NVergunst-ROBO
NVergunst-ROBO force-pushed the nv/ent-1686-1687-opencv-rtsps-hardening branch 3 times, most recently from 1635761 to 27824fe Compare August 21, 2026 15:17
dkosowski87
dkosowski87 previously approved these changes Aug 21, 2026
@NVergunst-ROBO

Copy link
Copy Markdown
Contributor Author

Follow-up from the lock discussion on #2776: dropped the process-wide _capture_open_lock here too.

USB/V4L2/file/plain RTSP opens are no longer serialized with RTSPS. The remaining lock is RTSPS-only (opencv_rtsps_tls_env). This PR's 5s timeout/stimeout still bounds that RTSPS lock hold.

Pushed in 93dad5c.

@dkosowski87

Copy link
Copy Markdown
Contributor

Current situation

At the current #2792 head, 93dad5cf, the broad lock around every VideoCapture open has been removed. [Removal commit](93dad5c)

The remaining lock is:

_opencv_rtsps_tls_lock = threading.Lock()

It applies only to RTSPS opens:

acquire RTSPS lock
→ save OPENCV_FFMPEG_CAPTURE_OPTIONS
→ install RTSPS options
→ call cv2.VideoCapture
→ restore previous environment
→ release lock

This guarantees that two simultaneous RTSPS opens cannot overwrite or restore each other’s options incorrectly.

The lock is held only while constructing VideoCapture; normal frame reading is not serialized. FFmpeg’s five-second socket timeout reduces how long an unreachable RTSPS camera can hold it, although it is not a strict total wall-clock opening timeout.

What remains unsafe

Non-RTSPS sources do not take the lock:

  • USB/V4L2 cameras
  • files
  • plain rtsp://
  • other URLs

This means a non-RTSPS OpenCV/FFmpeg capture can open while the RTSPS environment is temporarily installed and observe options intended for the RTSPS source.

For V4L2 this is generally irrelevant because it does not use FFmpeg. For files or plain RTSP using the FFmpeg backend, it can matter:

  • timeout may be inherited;
  • rtsp_transport=tcp may be inherited;
  • TLS settings may be parsed even though they are irrelevant;
  • externally supplied FFmpeg options may be unexpectedly merged.

There is also an independent concurrency issue: capture_process_stderr() replaces process-wide file descriptor 2. Concurrent opens can interfere with each other’s captured errors because that operation is not protected across non-RTSPS opens.

The current implementation explicitly accepts these risks so that a stuck RTSPS camera cannot block an unrelated USB camera or file from opening.

During the broad-lock implementation

An intermediate version introduced _capture_open_lock around every CV2VideoFrameProducer construction.

That solved both:

  • mixed RTSPS/non-RTSPS environment contamination;
  • overlapping stderr redirection.

However, it also meant:

one stuck network camera
→ blocks every other VideoCapture construction
→ including USB cameras, files and plain RTSP

Because those other backends do not necessarily have a bounded open timeout, one problematic source could stall all camera initialization or reconnection in the process.

The broad lock was therefore removed, returning to the current RTSPS-only compromise.

Long-term solution

The correct architectural fix is to stop using a process-wide environment variable for per-source configuration.

Preferred: a PyAV RTSPS producer

PyAV supports per-instance FFmpeg options:

av.open(
    url,
    options={
        "rtsp_transport": "tcp",
        "tls_verify": "1",
        "cafile": ca_path,
        "timeout": "5000000",
    },
    timeout=(5.0, read_timeout),
)

PyAV av.open documentation

This would provide:

  • TLS configuration scoped to one camera;
  • genuinely concurrent camera opens;
  • no environment mutation;
  • no TLS lock;
  • independent open and read timeouts;
  • different CA bundles or policies per source.

It is especially sensible because PyAV is already a repository dependency. The cost is implementing a VideoFrameProducer adapter for decoding, BGR conversion, metadata, grab/retrieve, release, and reconnection.

Wire opencv_rtsps_tls_env into CV2VideoFrameProducer so non-Jetson
OpenCV/FFmpeg RTSPS opens honor rfdm-injected CA bundle and TLS flags.

- Enter TLS env before capture_process_stderr (lock outside stderr redirect).
- Widen helper signatures to Union[str, int] for the producer call site.
- Document RTSPS-vs-RTSPS env isolation in the helper docstring.
Add module docstring for ROBOFLOW_RTSP_TLS_VALIDATION_FLAGS and CA bundle
env vars (self-hosted escape hatch). Assert env var is restored after open,
on capture failure, and untouched for int device indices.
OPENCV_FFMPEG_CAPTURE_OPTIONS and stderr dup2 are process-global, so concurrent
camera opens can clobber each other. Hold one lock around the open.
A process-wide lock around every producer would stall USB/V4L2/file
opens that have no FFmpeg timeout. Keep the RTSPS-only TLS env lock.
- Default FFmpeg stimeout and timeout (5s) to bound global TLS env lock hold.
- Pin cv2.CAP_FFMPEG for rtsps:// so TLS options cannot be ignored.
- Log when allow_self_signed (126) maps to full tls_verify=0 on FFmpeg.
A legacy env with only stimeout would skip timeout, which FFmpeg 5+ actually
uses. Fill whichever key is missing.
@NVergunst-ROBO
NVergunst-ROBO force-pushed the nv/ent-1686-1687-opencv-rtsps-hardening branch from 93dad5c to bb20812 Compare August 27, 2026 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants