Fix slow-starting stream playback fallbacks - #212
Conversation
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe proxy now separates startup and steady-state read timeouts for remux and chunked streams. Playback now uses typed direct/remux routes, mode-dependent ordering, per-route startup budgets, centralized failure handling, and updated route tests. ChangesStreaming proxy timeout handling
Playback route and startup handling
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Upstream
participant StreamingProxy
participant PlaybackClient
Upstream->>StreamingProxy: send delayed and subsequent chunks
StreamingProxy->>StreamingProxy: apply startup then steady timeout
StreamingProxy->>PlaybackClient: forward chunks or timeout result
sequenceDiagram
participant PlaybackAttempt
participant RouteHelper
participant PlaybackEngine
participant MediaElement
PlaybackAttempt->>RouteHelper: request startup or recovery routes
RouteHelper-->>PlaybackAttempt: return ordered direct/remux routes
PlaybackAttempt->>PlaybackEngine: try route with timeout budget
PlaybackEngine->>MediaElement: attach and load
PlaybackEngine-->>PlaybackAttempt: complete or fail attempt
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src-tauri/src/engine/stream_proxy.rs`:
- Around line 975-987: Update the read loop around received_data and
stream.next() so startup_read_timeout is measured against one fixed deadline
rather than restarted after each empty chunk. Recalculate each timeout from the
remaining time until that deadline while received_data is false, switch to
steady_read_timeout after the first non-empty chunk, and preserve the existing
timeout outcome.
In `@src/hooks/useStreamPlayer.ts`:
- Around line 1562-1567: Update the trailing fallback around tryNativePlayback
in useStreamPlayer so native-HLS URLs are excluded after the earlier native-HLS
attempt; retain this path only for non-HLS, mpegts, or unknown streams,
preserving existing fallback behavior for those cases.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: daf6fbb8-e59d-4a41-ae5c-de6da1e0c399
📒 Files selected for processing (3)
src-tauri/src/engine/stream_proxy.rssrc/hooks/useStreamPlayer.tstests/useStreamPlayer.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Define agent interfaces and implementations in TypeScript for agent systems
Use clear naming conventions for agent classes and methods
Document agent behavior and responsibilities in comments or docstrings
Implement error handling in agent methods
Files:
tests/useStreamPlayer.test.tssrc/hooks/useStreamPlayer.ts
src-tauri/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use Rust conventions: snake_case naming, 4-space indentation,
thiserrorfor error types, andserdefor serialization.
Files:
src-tauri/src/engine/stream_proxy.rs
src-tauri/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src-tauri/src/**/*.rs: Keep backend responsibilities organized intomodels,engine,commands,state.rs,error.rs, andlib.rsaccording to their documented roles.
Use event-driven scanning: emitscan://channel-resultevents per channel, with frontend event updates batched usingrequestAnimationFrame.
Files:
src-tauri/src/engine/stream_proxy.rs
src-tauri/src/engine/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Place core IPTV processing logic, including parsing, checking, ffmpeg, proxy, and resume functionality, in the engine module.
Files:
src-tauri/src/engine/stream_proxy.rs
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use strict TypeScript with no unused locals or parameters.
Files:
src/hooks/useStreamPlayer.ts
🔇 Additional comments (9)
src-tauri/src/engine/stream_proxy.rs (1)
30-32: LGTM!Also applies to: 778-790, 939-962, 998-1005, 1153-1153, 1414-1449, 1756-1794
src/hooks/useStreamPlayer.ts (6)
107-112: LGTM!
193-215: LGTM!
275-278: LGTM!
1233-1307: LGTM!
1309-1403: LGTM!
1511-1542: LGTM!tests/useStreamPlayer.test.ts (2)
11-11: LGTM!
66-104: LGTM!
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2141811c55
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src-tauri/src/engine/stream_proxy.rs (1)
975-987: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreserve the steady-state deadline across empty chunks.
After the first non-empty chunk, every
stream.next()call receives a freshsteady_read_timeout. Because empty chunks are skipped withcontinue, periodic empty frames can keep a stalled stream alive indefinitely and bypass the intended steady-state timeout. Track a fixed steady deadline and reset it only after forwarding non-empty data.Proposed fix
- let mut received_data = false; + let mut steady_deadline = None; let startup_deadline = tokio::time::Instant::now() + startup_read_timeout; loop { - let next_chunk = if received_data { - tokio::time::timeout(steady_read_timeout, stream.next()).await + let next_chunk = if let Some(deadline) = steady_deadline { + tokio::time::timeout_at(deadline, stream.next()).await } else { tokio::time::timeout_at(startup_deadline, stream.next()).await }; @@ - received_data = true; let permits = chunk.len().min(STREAM_PROXY_READ_AHEAD_BYTES) as u32; @@ if chunk_tx.send((chunk, permit)).await.is_err() { return StreamForwardOutcome::DownstreamClosed; } + steady_deadline = + Some(tokio::time::Instant::now() + steady_read_timeout);🤖 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 `@src-tauri/src/engine/stream_proxy.rs` around lines 975 - 987, Update the stream read loop containing received_data and startup_deadline to track a fixed steady-state deadline rather than recreating steady_read_timeout for each stream.next() call. Keep the deadline unchanged when empty chunks are skipped, and reset it only after successfully forwarding a non-empty chunk; preserve the startup deadline behavior before the first non-empty chunk.src/hooks/useStreamPlayer.ts (1)
1238-1247: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRoute initialization errors must resolve through the shared failure path.
The fallback orchestration calls
tryHlsPlayback/tryMpegtsPlaybackdirectly, so rejections before the existingtry/catchblocks skip recovery/fallback attempts. Wrap the early initialization and event registration for both routes inside the same failurefail()path.🤖 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 `@src/hooks/useStreamPlayer.ts` around lines 1238 - 1247, Ensure both tryHlsPlayback and tryMpegtsPlayback route initialization and event-registration failures through their existing shared fail() path, including dynamic imports, support checks, and player construction. Update the HLS flow at src/hooks/useStreamPlayer.ts lines 1238-1247 and the MPEG-TS flow at src/hooks/useStreamPlayer.ts lines 1315-1339; preserve normal successful playback and existing fallback behavior.
🤖 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.
Outside diff comments:
In `@src-tauri/src/engine/stream_proxy.rs`:
- Around line 975-987: Update the stream read loop containing received_data and
startup_deadline to track a fixed steady-state deadline rather than recreating
steady_read_timeout for each stream.next() call. Keep the deadline unchanged
when empty chunks are skipped, and reset it only after successfully forwarding a
non-empty chunk; preserve the startup deadline behavior before the first
non-empty chunk.
In `@src/hooks/useStreamPlayer.ts`:
- Around line 1238-1247: Ensure both tryHlsPlayback and tryMpegtsPlayback route
initialization and event-registration failures through their existing shared
fail() path, including dynamic imports, support checks, and player construction.
Update the HLS flow at src/hooks/useStreamPlayer.ts lines 1238-1247 and the
MPEG-TS flow at src/hooks/useStreamPlayer.ts lines 1315-1339; preserve normal
successful playback and existing fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3601a926-67d4-4cac-b626-024d8e69dbfd
📒 Files selected for processing (2)
src-tauri/src/engine/stream_proxy.rssrc/hooks/useStreamPlayer.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Build (macos-latest)
- GitHub Check: Build (windows-latest)
- GitHub Check: Parser Fuzz Smoke
- GitHub Check: Build (ubuntu-22.04)
- GitHub Check: Build (windows-latest)
🧰 Additional context used
📓 Path-based instructions (5)
src-tauri/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use Rust conventions: snake_case naming, 4-space indentation,
thiserrorfor error types, andserdefor serialization.
Files:
src-tauri/src/engine/stream_proxy.rs
src-tauri/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src-tauri/src/**/*.rs: Keep backend responsibilities organized intomodels,engine,commands,state.rs,error.rs, andlib.rsaccording to their documented roles.
Use event-driven scanning: emitscan://channel-resultevents per channel, with frontend event updates batched usingrequestAnimationFrame.
Files:
src-tauri/src/engine/stream_proxy.rs
src-tauri/src/engine/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Place core IPTV processing logic, including parsing, checking, ffmpeg, proxy, and resume functionality, in the engine module.
Files:
src-tauri/src/engine/stream_proxy.rs
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Define agent interfaces and implementations in TypeScript for agent systems
Use clear naming conventions for agent classes and methods
Document agent behavior and responsibilities in comments or docstrings
Implement error handling in agent methods
Files:
src/hooks/useStreamPlayer.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use strict TypeScript with no unused locals or parameters.
Files:
src/hooks/useStreamPlayer.ts
🔇 Additional comments (6)
src-tauri/src/engine/stream_proxy.rs (1)
30-32: LGTM!Also applies to: 778-790, 940-962, 1154-1154, 1412-1416, 1448-1450, 1758-1794
src/hooks/useStreamPlayer.ts (5)
107-112: LGTM!Also applies to: 193-215
1212-1218: LGTM!Also applies to: 1232-1237, 1250-1258, 1266-1291
1309-1314: LGTM!Also applies to: 1342-1370, 1372-1387
1486-1498: LGTM!Also applies to: 1511-1534, 1544-1559, 1562-1574
275-278: 🩺 Stability & AvailabilityNo need to change the aggregate loading budget.
LOADING_TIMEOUT_MSis 75s, while the non-HLS fallback path has a maximum sequential route budget of 65s (two MPEG-TS routes at 15s each, Xtream HLS at 15s, and native playback at 15s), leaving 10s for initialization and startup overhead.
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be28ba75bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/useStreamPlayer.ts (1)
1314-1333: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate the stream’s live/VOD mode into mpegts.js.
getMpegtsPlaybackRoutesreceivesisLive, buttryMpegtsPlaybackstill creates the player withisLive: true. VOD MPEG-TS routes should useisLive: falseso mpegts.js applies standard VOD buffering instead of live stream behavior.Proposed fix
- signal: AbortSignal, + signal: AbortSignal, + isLive: boolean, timeoutMs = MPEGTS_PLAYBACK_ROUTE_TIMEOUT_MS, @@ - isLive: true, + isLive, @@ - const mpegtsOk = await tryMpegtsPlayback(route.url, abortController.signal); + const mpegtsOk = await tryMpegtsPlayback( + route.url, + abortController.signal, + result.content_type === "live", + );🤖 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 `@src/hooks/useStreamPlayer.ts` around lines 1314 - 1333, Update tryMpegtsPlayback to accept the stream’s isLive value and pass it to mpegts.createPlayer instead of hardcoding isLive: true. Propagate the existing isLive argument from getMpegtsPlaybackRoutes through every tryMpegtsPlayback call, preserving live behavior while enabling standard VOD buffering.
🤖 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.
Outside diff comments:
In `@src/hooks/useStreamPlayer.ts`:
- Around line 1314-1333: Update tryMpegtsPlayback to accept the stream’s isLive
value and pass it to mpegts.createPlayer instead of hardcoding isLive: true.
Propagate the existing isLive argument from getMpegtsPlaybackRoutes through
every tryMpegtsPlayback call, preserving live behavior while enabling standard
VOD buffering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a62e7732-af40-4621-b0e0-68eb407541fc
📒 Files selected for processing (1)
src/hooks/useStreamPlayer.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: Build (windows-latest)
- GitHub Check: Build (windows-latest)
- GitHub Check: Build (macos-latest)
- GitHub Check: Build (ubuntu-22.04)
- GitHub Check: Rust Tests
- GitHub Check: Parser Fuzz Smoke
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Define agent interfaces and implementations in TypeScript for agent systems
Use clear naming conventions for agent classes and methods
Document agent behavior and responsibilities in comments or docstrings
Implement error handling in agent methods
Files:
src/hooks/useStreamPlayer.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use strict TypeScript with no unused locals or parameters.
Files:
src/hooks/useStreamPlayer.ts
🔇 Additional comments (1)
src/hooks/useStreamPlayer.ts (1)
109-112: LGTM!Also applies to: 193-215, 278-279, 1214-1214, 1233-1312, 1334-1412, 1498-1503, 1514-1514, 1560-1590
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c34826646f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
1 similar comment
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Root cause
Playback startup could spend its shared timeout on a speculative converted or remuxed route before reaching the original stream URL. The proxy also applied its six-second steady-state read timeout to the first media bytes, rejecting otherwise valid slow-starting providers.
Validation
Ref #211
Summary by CodeRabbit
Bug Fixes
Tests