Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ai/live: Add a minimum segment duration for new WHIP. #3451

Merged
merged 3 commits into from
Mar 26, 2025
Merged

Conversation

j0sh
Copy link
Collaborator

@j0sh j0sh commented Mar 21, 2025

Coalesces the rapid fire keyframes that we see sometimes

@github-actions github-actions bot added the go Pull requests that update Go code label Mar 21, 2025
@j0sh j0sh marked this pull request as ready for review March 26, 2025 00:38
Copy link

codecov bot commented Mar 26, 2025

Codecov Report

Attention: Patch coverage is 57.69231% with 11 lines in your changes missing coverage. Please review.

Project coverage is 30.39711%. Comparing base (6d8b5c3) to head (6cc2240).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
media/whip_server.go 0.00000% 8 Missing ⚠️
media/rtp_segmenter.go 83.33333% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #3451         +/-   ##
===================================================
+ Coverage   30.36565%   30.39711%   +0.03146%     
===================================================
  Files            151         151                 
  Lines          44524       44547         +23     
===================================================
+ Hits           13520       13541         +21     
- Misses         30214       30216          +2     
  Partials         790         790                 
Files with missing lines Coverage Δ
media/rtp_segmenter.go 86.33880% <83.33333%> (+3.80868%) ⬆️
media/whip_server.go 0.00000% <0.00000%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d8b5c3...6cc2240. Read the comment docs.

Files with missing lines Coverage Δ
media/rtp_segmenter.go 86.33880% <83.33333%> (+3.80868%) ⬆️
media/whip_server.go 0.00000% <0.00000%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@leszko leszko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. One question, what should we set as min segment duration in our infra?

@j0sh
Copy link
Collaborator Author

j0sh commented Mar 26, 2025

what should we set as min segment duration in our infra

@leszko The default in this PR is 1 second, I think that is fine to leave as-is (so, no infra changes needed). We send keyframe requests every 2 seconds so our actual segment duration will be closer to 2 seconds.

This is mostly meant to mitigate potential issues where multiple segments are emitted in a bursty fashion, eg frames buffered then blasted out within a few milliseconds. This causes us to have multiple (>3) segments in-flight, which the slow orchestrator checker doesn't like.

I have also tested making the min segment duration longer (eg, 10 seconds) which means we coalesce multiple keyframes into a single segment ... I don't think that is necessary right now, but we have that option if we need it later on. (things are still realtime since we don't buffer segments)

@j0sh j0sh enabled auto-merge (squash) March 26, 2025 16:41
@j0sh j0sh merged commit 4802231 into master Mar 26, 2025
15 of 16 checks passed
@j0sh j0sh deleted the ja/min-seg-dur branch March 26, 2025 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants