Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
LPMS bottleneck fix #141
LPMS bottleneck fix #141
Changes from all commits
f1f732d
db43fc7
72ea7e9
e34a473
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the change from converting to yuv420p before hwdownload to converting to nv12 after hwdownload?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Old way was actually broken - it only worked with the
npp
scaler which we phased out in favor of CUDA before the original GPU PR was even merged. See livepeer/go-livepeer#951This seems to work better now that we're using cuvid as a decoder, but I have to check that it actually fixes livepeer/go-livepeer#951
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, in what scenarios wouldn't we want to enforce IDR frames or closed GOPs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't set it if the user passes in their own options. Practical scenarios for open GOP and non-intra I frames include non-segmented VOD or real time streaming where you want to take advantage of a larger range of pictures for the bitrate savings.
On checking again, nvenc doesn't actually the cgop option, so it's useless for us now. Will remove. It was a leftover from when trying to kludge x264 into doing persistent transcode sessions. We aren't anymore, so this option is no longer needed.
The
forced-idr
option might not be needed as well...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So
forced-idr
is in fact needed here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the
force-idr
option is required - without it, we get decoder errors at the beginning of playback because SPS/PPS extradata isn't always is emitted for the first frame of new segments, which we signal via an explicit keyframe. This option forces that.Ideally this option wouldn't be needed at all; ffmpeg itself should but smart enough to emit extradata for the first frame after a flush; I may add that in.
Also just checked and it seems that disabling this option doesn't lead to any unit test failures - we really should capture this behavior in tests. Will make a note to correct that.