BYOC: add streaming to BYOC#3841
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3841 +/- ##
===================================================
+ Coverage 31.55325% 31.90751% +0.35426%
===================================================
Files 163 169 +6
Lines 39324 41216 +1892
===================================================
+ Hits 12408 13151 +743
- Misses 26022 27079 +1057
- Partials 894 986 +92
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
@ad-astra-video I’ve completed a full review of this pull request, and it’s very close to being ready to merge. Since much of the code was new to me, I had several questions and comments, some related to code outside the scope of this PR, which I captured in this document to avoid adding too much noise here. Let’s do a quick review together so I can leave more targeted feedback directly on the PR and we can move toward merging.
byoc/byoc.go
Outdated
|
|
||
| //ensure streamRequest is not nil or empty to avoid json unmarshal issues on Orchestrator failover | ||
| //sends the request bytes to next Orchestrator | ||
| if streamRequest == nil || len(streamRequest) == 0 { |
There was a problem hiding this comment.
Small nit but wouldn't this work given that len of nil byte gives true? Not to important but just something I noticed.
| if streamRequest == nil || len(streamRequest) == 0 { | |
| // Ensure a non-empty request body for failover | |
| if len(streamRequest) == 0 { | |
| streamRequest = []byte("{}") | |
| } |
There was a problem hiding this comment.
Feel free to ignore this one very minor. Just something I noticed.
rickstaa
left a comment
rickstaa
left a comment
There was a problem hiding this comment.
@ad-astra-video general thing I found during my review outside of your changes.
It might still make sense to add a defer cancel() after creating the context. While it will eventually time out anyway, explicitly canceling ensures the cancellation happens immediately once the function returns and can help free resources sooner.
respCtx, cancel := context.WithTimeout(
ctx,
time.Duration(orchJob.Req.Timeout)*time.Second,
)
defer cancel()Feel free to ignore as this is a nit.
byoc/stream_orchestrator.go
Outdated
| if !exists { | ||
| req, err := http.NewRequestWithContext(ctx, "POST", orchJob.Req.CapabilityUrl+"/stream/stop", nil) | ||
| // set the headers | ||
| resp, err = sendReqWithTimeout(req, time.Duration(orchJob.Req.Timeout)*time.Second) |
There was a problem hiding this comment.
@ad-astra-video does using resp and w in this function leads to race conditions as these are passed from the handler scope?
There was a problem hiding this comment.
@ad-astra-video thanks for addressing my comments in the Notion doc. I’ve reviewed the latest changes and everything looks good for a merge.
As far as I can tell, this pull request should not introduce any side effects to the transcoding or live pipelines since it lives in its own namespace. While it does share some state, those components are intentionally shared across pipelines and are considered stable. The test you added should also flag any regressions caused by future changes in this area.
I did leave a few minor nits, but feel free to merge as-is.
Future improvements
For future reference, based on my discussion with Brad, these are areas he was considering exploring in follow-up pull requests, rather than firm commitments:
- Was considering revisit creating a clearer separation between the legacy
batch/batch-streamingpipelines and the newerbyoc.livepipeline so it’s more obvious which components are shared across all pipelines. - Discussed potentially abstracting the orchestrator selection logic so it can be reused across these three byoc pipelines (see byoc/job_gateway.go#L67 and
byoc/stream_gateway.go#L178) - Mentioned exploring whether
batch-streamingcan leverage the newbyoc.livepipeline. - Considers looking into simplifying the logic at
byoc/job_orchestrator.go#L222
injob_orchestrator.go, which currently combines multiple responsibilities such as job proxying, periodic payment debits, and balance enforcement. - Discussed potentially separating payment logic in the orchestrator to simplify the code:
byoc/stream_orchestrator.go#L181 - Brad will add developer documentation describing the exact workings of BYOC and how it differs from Live.
- Add WHEP support at a later stage.
- May also revisit the weaker job/token credit verification logic (see verifyJobCreds and verifyTokenCreds ) and see if it can be reused to reduce divergence.
Further reduction of duplicate code
@j0sh, as discussed, @ad-astra-video duplicated some parts of the live pipeline to unblock onboarding additional demand partners and allow progress to continue independently.
We identified a few areas where INC could help reduce code duplication:
- Would it make sense to make the methods in auth.go exportable so BYOC can reuse them and remove the BYOC-specific auth file?
- Would it make sense to move the trickle logic in live_video_video.go into a shared
trickle.gopackage? This would allow the community to benefit from INC’s work while reducing duplication. - BYOC also uses several methods in the byoc.utils file that are copied from the
livepipeline. Are there any of these methods that make sense to move into a shared helper so they can be reused, given that they don’t change often?
Thanks for your consideration.
Co-authored-by: PSchroedl <peter_schroedl@me.com>
3 participants
What does this pull request do? Explain your changes. (required)
Final PR after splitting original PR #3727 into smaller PRs and refactoring BYOC into separate folder
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 mix of the following:
Control and Events channels are created for every stream.
Streams are created with a POST request to
/process/stream/startthat 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 thestream_id. Thestream_idis an integral part of the URLs provided to interact with the stream and is combined with a provided stream name in the /process/stream/start request.Streams are stopped with a POST request to
/process/stream/{stream_id}/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.Orchestrators use
/ai/streamprefix for start/stop/update and payment. All outputs use trickle to send the outputs received from the workers to the Gateway.Specific updates (required)
byocfolderserverpackage as needed to allow the streaming endpoints to workHow did you test each of these updates (required)
synctestand all byoc tests are run with-raceenabled in `./test.shDoes this pull request close any open issues?
No
Checklist:
makeruns successfully./test.shpass