Skip to content

add ffmpeg-copyts option #3404

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
Dec 16, 2020
Merged

add ffmpeg-copyts option #3404

merged 2 commits into from
Dec 16, 2020

Conversation

Billy2011
Copy link
Contributor

hls-multi streams occasionally have an input offset between the audio and video streams and the sound is then out of sync. With copyts, the offset is not changed when outputting to the player so that the sound is in sync.

@Billy2011 Billy2011 force-pushed the ffmpeg-copyts-py3 branch 2 times, most recently from 8d217d6 to 9d04f6a Compare December 9, 2020 16:38
bastimeyer
bastimeyer reviewed Dec 9, 2020
View reviewed changes
@back-to back-to mentioned this pull request Dec 13, 2020
Merged
@Billy2011 Billy2011 force-pushed the ffmpeg-copyts-py3 branch 2 times, most recently from 5e8c6d5 to 9aef6f3 Compare December 15, 2020 10:48
back-to
back-to reviewed Dec 15, 2020
View reviewed changes
Copy link
Collaborator

@back-to back-to left a comment

Choose a reason for hiding this comment

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

could you add some tests for

session.options.set("ffmpeg-copyts", False)
session.options.set("ffmpeg-copyts", True)

similar to https://github.com/streamlink/streamlink/blob/master/tests/streams/test_ffmpegmux.py#L56-L117

@Billy2011 @bastimeyer
add ffmpeg-copyts option
3115a4c 
Co-authored-by: Sebastian Meyer <mail@bastimeyer.de>

Co-authored-by: Sebastian Meyer <mail@bastimeyer.de>
bastimeyer
bastimeyer requested changes Dec 15, 2020
View reviewed changes
Copy link
Member

@bastimeyer bastimeyer left a comment

Choose a reason for hiding this comment

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

Please move all the definitions of ffmpeg-copyts up, so that it comes before ffmpeg-start-at-zero, as start-at-zero depends on copyts. In the Session, FFMPEGMuxer, argparser and cli.main.

@Billy2011
Copy link
Contributor Author

as start-at-zero depends on copyts

Please why, I see no dependence?

@bastimeyer
Copy link
Member

streamlink/src/streamlink/stream/ffmpegmux.py

Lines 114 to 117 in ca1b966

if copyts:
self._cmd.extend(["-copyts"])
if start_at_zero:
self._cmd.extend(["-start_at_zero"])

@Billy2011
Copy link
Contributor Author

start_at_zero = session.options.get("ffmpeg-start-at-zero") or options.pop("start_at_zero", False)
copyts = session.options.get("ffmpeg-copyts") or options.pop("copyts", False)

It doesn’t matter which option comes first.

@back-to
Copy link
Collaborator

back-to commented Dec 15, 2020

it is basically just style

git diff, git history would look better then now

https://github.com/streamlink/streamlink/pull/3404/files#diff-04516d5f6b73bc1d25a0bc7385dc2b3e1993578ff01d3a34ec040a271d71b73d

@bastimeyer
Copy link
Member

it is basically just style

What's important here is the order of the CLI args in the docs / man page / help text. And since that definitely should be corrected, it doesn't make sense having the other code lines in a different order.

@Billy2011
Copy link
Contributor Author

Have a good excuse ;)

@back-to back-to merged commit b2748a7 into streamlink:master Dec 16, 2020
Billy2011 added a commit to Billy2011/streamlink-27 that referenced this pull request Dec 17, 2020
@Billy2011
[PATCH] stream.ffmpegmux: add --ffmpeg-copyts option (streamlink#3404)
09f35fd
Billy2011 added a commit to Billy2011/streamlink-27 that referenced this pull request Dec 17, 2020
@Billy2011
[PATCH] stream.ffmpegmux: add --ffmpeg-copyts option (streamlink#3404)
a5653e6
@Billy2011 Billy2011 deleted the ffmpeg-copyts-py3 branch December 24, 2020 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@back-to back-to back-to left review comments

@bastimeyer bastimeyer bastimeyer approved these changes

Assignees
No one assigned
Labels
stream: ffmpegmux
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Billy2011 @bastimeyer @back-to