fix: proxy playlist parsing logic#1205
Merged
Merged
Conversation
WalkthroughThis PR replaces grafov/m3u8 with an internal HLS decoder using gohlslib. It adds DecodeMultivariant with Twitch-specific normalization, updates call sites (platform, exec/download, proxy tests) to use *hls.Multivariant, and includes tests plus audio-only archive/live test additions. ChangesHLS Library Migration
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/exec/util.go (1)
61-69: 💤 Low valueInconsistent logging and potential information leak.
Using
fmt.Printlnfor debug output is inconsistent with the rest of the codebase, which uses thezerolog/logpackage. Additionally, printing the raw HLS playlist to stdout may expose sensitive information, as playlist variant URIs can contain authentication tokens or signed parameters.Consider using
log.Debug().Msg(string(bodyBytes))instead, or remove this debug output entirely if it was only needed during development.♻️ Proposed fix to use structured logging
- fmt.Println("===") - // print body bodyBytes, err := io.ReadAll(resp.Body) if err != nil { log.Error().Err(err).Msg("error reading response body for Twitch HLS proxy server test") return nil, false } - fmt.Println(string(bodyBytes)) - fmt.Println("===") + log.Debug().Str("playlist_body", string(bodyBytes)).Msg("Twitch HLS proxy response")🤖 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 `@internal/exec/util.go` around lines 61 - 69, Replace the fmt.Println debug output that prints the raw HLS playlist (the io.ReadAll(resp.Body) / bodyBytes printing) with structured zerolog logging or remove it to avoid leaking auth tokens; specifically, in the code around the io.ReadAll(resp.Body) call and the subsequent fmt.Println statements, stop writing to stdout and instead call log.Debug().Msg(string(bodyBytes)) if you need the content for troubleshooting, or drop the debug output entirely, keeping the existing log.Error().Err(err).Msg(...) for read failures.
🤖 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 `@internal/exec/util.go`:
- Around line 63-71: Unbounded io.ReadAll on resp.Body can exhaust memory before
hls.DecodeMultivariant's internal limit runs; wrap resp.Body with io.LimitReader
(using the same maxPlaylistSize constant used by internal/hls/playlist.go) and
read from that limited reader when producing bodyBytes and when passing to
hls.DecodeMultivariant so both the debug output and DecodeMultivariant enforce
the same size cap (adjust references in internal/exec/util.go where bodyBytes,
resp.Body and hls.DecodeMultivariant are used).
---
Nitpick comments:
In `@internal/exec/util.go`:
- Around line 61-69: Replace the fmt.Println debug output that prints the raw
HLS playlist (the io.ReadAll(resp.Body) / bodyBytes printing) with structured
zerolog logging or remove it to avoid leaking auth tokens; specifically, in the
code around the io.ReadAll(resp.Body) call and the subsequent fmt.Println
statements, stop writing to stdout and instead call
log.Debug().Msg(string(bodyBytes)) if you need the content for troubleshooting,
or drop the debug output entirely, keeping the existing
log.Error().Err(err).Msg(...) for read failures.
🪄 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: f77fa68b-195e-46a6-b871-8662b3492aa4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
go.modinternal/exec/exec.gointernal/exec/util.gointernal/hls/playlist.gointernal/hls/playlist_test.gointernal/platform/twitch.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/archive/archive_test.go (1)
66-86: ⚡ Quick winExtract duplicated helper to shared test package.
The
assertAudioOnlyFilehelper is duplicated ininternal/live/live_test.go(lines 242-262). Extract this to a shared test helper (e.g.,tests/shared/media_helpers.goor similar) to avoid duplication and ensure consistent behavior across test suites.♻️ Suggested refactor
Create a new file
tests/shared/media_helpers.go:package tests_shared import ( "testing" "github.com/stretchr/testify/assert" internalExec "github.com/zibbp/ganymede/internal/exec" ) func AssertAudioOnlyFile(t *testing.T, path string) { t.Helper() probeData, err := internalExec.GetFfprobeVideoData(t.Context(), path) assert.NoError(t, err, "Failed to probe archived media") assert.NotNil(t, probeData, "Expected ffprobe data for archived media") audioStreams := 0 videoStreams := 0 for _, stream := range probeData.Streams { switch stream.CodecType { case "audio": audioStreams++ case "video": videoStreams++ } } assert.Greater(t, audioStreams, 0, "Archived media should contain at least one audio stream") assert.Zero(t, videoStreams, "Archived media should not contain video streams") }Then replace both local helpers with calls to
tests_shared.AssertAudioOnlyFile(t, 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 `@internal/archive/archive_test.go` around lines 66 - 86, The helper assertAudioOnlyFile is duplicated; extract it into a shared test helper named AssertAudioOnlyFile in a new tests_shared package, implementing the same logic (use internalExec.GetFfprobeVideoData, t.Helper(), assert.NoError/NotNil, count audio/video streams and assert results) and then remove the local assertAudioOnlyFile definitions and replace calls with tests_shared.AssertAudioOnlyFile(t, path) in the test suites that currently duplicate that logic.
🤖 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.
Nitpick comments:
In `@internal/archive/archive_test.go`:
- Around line 66-86: The helper assertAudioOnlyFile is duplicated; extract it
into a shared test helper named AssertAudioOnlyFile in a new tests_shared
package, implementing the same logic (use internalExec.GetFfprobeVideoData,
t.Helper(), assert.NoError/NotNil, count audio/video streams and assert results)
and then remove the local assertAudioOnlyFile definitions and replace calls with
tests_shared.AssertAudioOnlyFile(t, path) in the test suites that currently
duplicate that logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8642675-7c74-46f4-981e-ee089f537a62
📒 Files selected for processing (4)
internal/archive/archive_test.gointernal/exec/util.gointernal/hls/playlist.gointernal/live/live_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/exec/util.go
- internal/hls/playlist.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
VIDEOattribute