Skip to content

stream.ffmpegmux: fix copy-to-pipe race condition #5162

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 14, 2023
Merged

stream.ffmpegmux: fix copy-to-pipe race condition #5162

merged 2 commits into from
Feb 14, 2023

Conversation

bastimeyer
Copy link
Member

Make sure that each substream's buffer always gets fully emptied when closing the stream, so that no data is missing when muxing the output stream.

TODO: properly refactor FFMPEGMuxer class with full test coverage

Fixes #5159

The second commit fixes another race condition in the NamedPipePosix implementation when attempting to close the file descriptor of a broken named pipe where the fifo wouldn't get unlinked in the filesystem.

As mentioned in #5159 (comment), I will have a look at properly refactoring/rewriting the FFMPEGMuxer class some time later.

Pipe-copying now ends simultaneously and no data should be missing in the muxed output stream:

$ streamlink -l debug -o /tmp/foo.mkv 'https://vimeo.com/channels/staffpicks/790905324' best
...
[stream.dash][debug] Download of segment: 0c433f4a.mp4 complete
[stream.dash][debug] Download of segment: 9ae75d32.mp4 complete
...
[stream.dash][debug] Download of segment: 0c433f4a.mp4 complete
[stream.dash][debug] Download of segment: 9ae75d32.mp4 complete
[stream.segmented][debug] Closing writer thread
[stream.dash][debug] Download of segment: 0c433f4a.mp4 complete
[stream.dash][debug] Download of segment: 0c433f4a.mp4 complete
...
[stream.dash][debug] Download of segment: 0c433f4a.mp4 complete
[stream.segmented][debug] Closing writer thread
[stream.ffmpegmux][debug] Pipe copy complete: /tmp/streamlinkpipe-26548-1-3723
[stream.ffmpegmux][debug] Pipe copy complete: /tmp/streamlinkpipe-26548-2-7524
[stream.ffmpegmux][debug] Closing ffmpeg thread
[stream.ffmpegmux][debug] Closed all the substreams
[cli][info] Stream ended
[cli][info] Closing currently open stream...

$ ffmpeg -i /tmp/foo.mkv -c copy /tmp/foo.mp4
...

$ ffprobe -v error -of json -show_streams /tmp/foo.mp4 | jq -r .streams[].duration
914.916000
914.922333

@bastimeyer
stream.ffmpegmux: fix copy-to-pipe race condition
9a2727f 
Make sure that each substream's buffer always gets fully emptied
when closing the stream, so that no data is missing when muxing
the output stream.

TODO: properly refactor FFMPEGMuxer class with full test coverage
@bastimeyer bastimeyer force-pushed the stream/ffmpegmux/fix-copy-to-pipe branch from 213ebd6 to c4f7e80 Compare February 14, 2023 19:15
@gravyboat gravyboat merged commit 498efd5 into streamlink:master Feb 14, 2023
@bastimeyer bastimeyer deleted the stream/ffmpegmux/fix-copy-to-pipe branch February 14, 2023 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug stream: ffmpegmux
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

stream.ffmpegmux: race condition when copying data to named pipe
2 participants
@bastimeyer @gravyboat