-
Notifications
You must be signed in to change notification settings - Fork 72
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
Use ffmpeg parser for H.264 #431
Conversation
Fixes a number of things including a LPMS crash, choppy video quality, green screens during rotation, inconsistent frame counts vs software decoding, etc. This is a massive change: we can no longer add outputs up front due to the ffmpeg hwaccel API, so we have to wait until we receive a decoded video frame in order to add outputs. This also means properly queuing up audio and draining things in the same order.
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.
I skimmed through the PR and looks good. Saying that, I don't have enough LPMS knowledge to review it in depth, so if you need a "better" review, then you can wait for approves from some other folks.
ffmpeg/nvidia_test.go
Outdated
fname := dir + "/mpeg2.ts" | ||
oname := dir + "/out.ts" | ||
fname := dir + "/test.flv" | ||
//oname := dir + "/out.ts" |
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.
Nit: Remove commented-out code
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.
Thanks for the spot, fixed and cleaned up in 9e51cee
@@ -603,6 +603,7 @@ func TestNvidia_API_AlternatingTimestamps(t *testing.T) { | |||
tc := NewTranscoder() | |||
idx := []int{1, 0, 3, 2} | |||
for _, i := range idx { | |||
// TODO this breaks with nvidia acceleration on the input! |
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.
Note that this is not a new problem with this PR; the old code had the same issue
Maybe @emranemran you could take a look at the C code? Can't remember if you've reviewed much of lpms in the past.. |
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.
Similar to Rafal I'm afraid I don't know enough about this stuff, maybe we could do a knowledge transfer call or something?
Fixes a number of things with the nvidia built-in parser (cuvid) including a LPMS crash, choppy video quality, green screen flashing during rotation, inconsistent frame counts vs software decoding, etc. We also apparently gained GPU support for MPEG2 decoding.
This is a massive change: we can no longer add outputs up front due to the ffmpeg hwaccel API, so we have to wait until we receive a decoded video frame in order to add outputs. This also means properly queuing up audio and draining things in the same order.
This also unfortunately adds ~13 MB of new samples to the repo for test cases, since these are hard to generate synthetically.