Revert "Revert "BYOC: add streaming"" #3807
Conversation
This reverts commit daf4126.
pschroedl
requested review from
ad-astra-video,
j0sh,
leszko,
rickstaa and
victorges
github-actions
bot
added
go
|
Per @ad-astra-video : Adds configurable streaming for BYOC entrypoint to go-livepeer. Uses trickle protocol to handle streaming for similar entrypoints and outputs from go-livepeer as live-video-to-video. Streams can be any or a mix of the following: video ingress via WHIP (with Gateway) or RTMP (with MediaMTX) Streams are created with a POST request to /ai/stream/start that will start the stream and reserve the capacity with an Orchestrator that is providing the BYOC capability. If video ingress is enabled, the client should then start a stream with WHIP or RTMP to the provided ingress URLs provided in the response. URLs for egress video, data, updates (control) and events are also included in the response as well as the stream_id. The stream_id is an integral part of the URLs provided to interact with the stream and is combined with a provided stream name in the /ai/stream/start request. Streams are stopped with a POST request to /ai/stream/stop. Orchestrators and Gateways track payment balance and the Gateway adjusts to the Orchestrators provided balance in new JobTokens provided at each payment interval every minute. Orchestrators will shutdown a stream when payment balance is zero. Specific updates (required) Add job_stream.go and job_stream_test.go Used byoc-stream to test end to end: https://github.com/ad-astra-video/livepeer-app-pipelines/tree/main/byoc-stream |
|
Original PR for reference/comments : #3727 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3807 +/- ##
===================================================
+ Coverage 31.67941% 32.35002% +0.67061%
===================================================
Files 159 160 +1
Lines 38978 40306 +1328
===================================================
+ Hits 12348 13039 +691
- Misses 25735 26275 +540
- Partials 895 992 +97
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
Leaving a note here, review is still in progress and will finish up within the next few days |
…tart trickle channels if not needed
server/job_stream_test.go
Outdated
|
|
||
| //kick the second Orchestrator | ||
| go func() { | ||
| time.Sleep(200 * time.Millisecond) |
There was a problem hiding this comment.
I recommend using synctest for all timing dependent tests, it makes things run much faster and we don't really want to make the test suite slower than it already is. It'll also surface some types of concurrency issues as long as the tests are run with-race (not sure if they already are, but if not, also recommended)
There was a problem hiding this comment.
I tried and could not get synctest to work. I don't know how to get it to work with the httptest server seems like. If this is mandatory can I ask I look at converting in a follow up PR?
That said, I was able to remove most of the time.Sleep instances in job_stream_test.go. Tests in job_stream_test.go takes 500-600 milliseconds to run locally.
I also ran the two most important tests using -race and added some locks around updates in liveParams. Not sure all the locks will catch actual race errors but better safe than have to deep dive into it. These two tests were added to continue to use -race detector in test.sh as well.
There was a problem hiding this comment.
Update: got all tests in job_stream_test.go to work with synctest. Ran all locally with count=10 successfully with go test -timeout 30s -run _BYOC$ github.com/livepeer/go-livepeer/server -count=10
server/job_stream.go
Outdated
|
|
||
| // startStreamProcessingFunc is an alias for startStreamProcessing that can be overridden in tests | ||
| // to avoid starting up actual stream processing | ||
| var startStreamProcessingFunc = startStreamProcessing |
There was a problem hiding this comment.
FWIW, we have the same pattern in discovery and it's been horrible for test correctness, I've been trying to unwind our use of that so we can have cleaner tests that pass more reliably.
If it's not easy to override whatever the blocker is here then I'd suggest putting startStreamProcessingFunc in some struct that can be set on a per-test basis with startStreamProcessing as the default for normal use.
There was a problem hiding this comment.
I was able to get around this by making sure to pass no URL to the trickle URLs for subscriber/publisher/data channels.
…d use synctest on most of it
Reverts the revert of BYOC: Add Streaming
#3804