Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3396 +/- ##
===================================================
- Coverage 32.19210% 32.16198% -0.03012%
===================================================
Files 147 147
Lines 40687 40722 +35
===================================================
- Hits 13098 13097 -1
- Misses 26816 26852 +36
Partials 773 773
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
leszko
left a comment
There was a problem hiding this comment.
I think it looks good. Two things I'd check before merging and deploying to prod:
- How much CPU/MEM usage of mediamtx pod increases with each viewer?
We have the following mediamtx kubernetes resources set, maybe we'll need to increase it.
resources:
limits:
memory: 4Gi
requests:
memory: 4Gi- If one ffmpeg RTMP command fails, does it still work? E.g. if you can't push it into studio, will the push to MediaMTX work? I think it should, but I'd double check because these
multiwriterand all theseio.writerin golang behaves weirdly sometimes.
Thanks Max 🙏
server/ai_live_video.go
Outdated
| err = errors.New("unknown error") | ||
| } | ||
| clog.Errorf(ctx, "LPMS panic err=%v", err) | ||
| params.liveParams.stopPipeline(fmt.Errorf("LPMS panic %w", err)) |
There was a problem hiding this comment.
Could we add a graph to our AI dashboard in grafana to track this?
There was a problem hiding this comment.
I'll raise a ticket for that 👍
server/ai_live_video.go
Outdated
| } | ||
|
|
||
| cmd := exec.Command("ffmpeg", | ||
| "-i", "pipe:0", |
There was a problem hiding this comment.
If the thread that pushes segments into the pipe:0 crashes, does this ffmpeg process die? what does recovery look like then?
There was a problem hiding this comment.
This was from the previous logic, and yep that's right, if the thread pushing segments stops then stopPipeline would have been called here and so everything shuts down then the input stream can reconnect and try again.
server/ai_live_video.go
Outdated
| // return | ||
| } | ||
| clog.Infof(ctx, "Process output: %s", output) | ||
| time.Sleep(5 * time.Second) |
There was a problem hiding this comment.
What is this 5s sleep for?
|
@leszko Very good point about the multiwriter, it does indeed fail if there are issues with either ffmpeg output, I've changed it now to a custom implementation which only returns an error if all writes fail. I'll check out the cpu usage in staging 👍 |
|
This breaks MediaMTX playback during local testing with the default setup unless a separate rtmp output URL is specified, because there are two output streams of the same name going into MediaMTX ... @mjh1 can you fix this? |
|
Also I believe we should be careful with the multi writer here because one blocked write can still stall the others; ideally they would be asynchronous (which also implies doing some buffer management). Not sure if this PR is meant to be a temp fix during the latency investigations though (in which case we can probably live with this blocking risk as long as things are reverted later on) |
We want to try direct playback from MediaMTX to see how much startup time is saved from avoiding studio stream startup.