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

Write piped output periodically, rather than waiting for stdin to close #25

Merged
merged 4 commits into from
Jul 28, 2023

Conversation

thomshutt
Copy link
Contributor

During a livestream, Mist currently pipes updates to the manifest file continuously to the uploader binary but only closes the pipe once the stream is complete.

This means that we don't get a manifest file written until the stream finishes, which:

  • Makes debugging and recovery more difficult if something goes wrong with the stream partway through
  • Means a recording can't be played back until it's complete
  • Stops us building features like clipping on top of an in-progress livestream

@codecov
Copy link

codecov bot commented Jul 22, 2023

Codecov Report

Merging #25 (78de1e7) into main (5457805) will increase coverage by 31.57895%.
The diff coverage is 31.57895%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##               main         #25          +/-   ##
===================================================
+ Coverage   0.00000%   31.57895%   +31.57895%     
===================================================
  Files             1           2           +1     
  Lines            74          76           +2     
===================================================
+ Hits              0          24          +24     
+ Misses           74          47          -27     
- Partials          0           5           +5     
Files Changed Coverage Δ
catalyst-uploader.go 0.00000% <0.00000%> (ø)
core/uploader.go 58.53659% <58.53659%> (ø)

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 5457805...78de1e7. Read the comment docs.

Files Changed Coverage Δ
catalyst-uploader.go 0.00000% <0.00000%> (ø)
core/uploader.go 58.53659% <58.53659%> (ø)

@thomshutt thomshutt requested a review from iameli July 24, 2023 09:39
Copy link
Member

@victorges victorges left a comment

Choose a reason for hiding this comment

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

LGTM! Tho not sure I understood what is the Mist interface we're implementing for for the incremental file updates

catalyst-uploader.go Show resolved Hide resolved
catalyst-uploader.go Show resolved Hide resolved
core/uploader.go Show resolved Hide resolved
@thomshutt
Copy link
Contributor Author

Tho not sure I understood what is the Mist interface we're implementing for for the incremental file updates
When Mist has a file to upload, it calls this binary and pipes in whatever it's trying to write. In practice, that's either:

  • Video/Audio Segments that should upload in 1s
  • Manifests that for a livestream will be appended to for the entirety of the livestream. This is the case we're really interested in with this change, since in the current iteration they're only written once the stream finished and Mist closes the input pipe to the uploader binary.

@thomshutt thomshutt merged commit ce16a69 into main Jul 28, 2023
10 checks passed
@thomshutt thomshutt deleted the incremental-writing branch July 28, 2023 11:27
@victorges
Copy link
Member

@thomshutt cool thanks for the explanation! I think the current implementation looks good for the use case while being simpler than handling background tickers or timers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants