new whip server #3432
new whip server #3432
Conversation
github-actions
bot
added
dependencies
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3432 +/- ##
===================================================
- Coverage 32.16679% 32.04433% -0.12246%
===================================================
Files 147 150 +3
Lines 41033 41867 +834
===================================================
+ Hits 13199 13416 +217
- Misses 27058 27664 +606
- Partials 776 787 +11
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
Added the ability to disable WHIP ingest entirely by omitting the Ideally we'd have a CLI argument like |
There was a problem hiding this comment.
Great work @j0sh Added some comments, mostly to the ai_mediaserver.go part. I was not able to review in depth the media part, in particular the whip_server.go.
My suggestion is to address the comments and get it merged, then we can test it in action:
- I'd not use env vars, but move it to flags (as we do with everything else)
- It'd be super beneficial to describe how you test this whip endpoint locally (can be a README or even some short Notion doc)
| ls.HTTPMux.Handle("/live/video-to-video/smoketest", ls.SmokeTestLiveVideo()) | ||
|
|
||
| // Configure WHIP ingest only if an addr is specified. | ||
| // TODO use a proper cli flag | ||
| if os.Getenv("LIVE_AI_WHIP_ADDR") != "" { |
There was a problem hiding this comment.
How about moving this to flags? I think we don't use anywhere the env variables directly in go-livepeer, but just adding more flags.
There was a problem hiding this comment.
Yes it has been mentioned in several places this can be done as a follow up to this PR
| pipeline = authResp.Pipeline | ||
| } | ||
|
|
||
| if len(authResp.paramsMap) > 0 { |
There was a problem hiding this comment.
I see there is a lot of code copied from StartLiveVideo(), I'd consider refactoring it and re-use. It may be okeyish to just copy-paste the code like you did, but I think there is some functionality missing (e.g. I don't see sending stream_trace) into Kafka, which may make it more difficult to analyze.
There was a problem hiding this comment.
Can add the stream_trace (I think that was added after we wrote this code) but wanted to avoid modifying existing ingest code paths - they are too different and the risk was too high of breaking something
There was a problem hiding this comment.
ok, then let's just add the stream_trace.
There was a problem hiding this comment.
Added stream_trace and some other missing metrics in 7236f83
media/whip_server.go
Outdated
| PayloadType: 105, | ||
| }, | ||
|
|
||
| // TODO verify 42e01f won't produle bframes |
There was a problem hiding this comment.
Let's remove this before merging
There was a problem hiding this comment.
Sure, this was part of the default set of Pion codecs but I don't think this part even does what we need it to right now - the remote SDPs the browser is setting don't look as constrained as I was expecting. Needs more investigation
Introduces a WHIP endpoint at
/live/video-to-video/{streamid}/whipThis strictly affects ingest only; it does not touch selection, trickle or anything beyond those.
There should also be no changes to the existing MediaMTX whip / rtmp ingest. The old ingest is safe to run alongside this and there should be no behavioral changes to that. The one modification is to tighten up request methods in the HTTP handler (adding
POSTprefixes etc) to prevent route conflicts with the new WHIP endpoints.New Configuration Options
Currently implemented directly as environment variables.
Other Changes
Added a
clog.Infofunction for a slog-style logging APINot Yet Implemented
These todos are all in Linear for later follow-up.