Fix CUVID crash on resolution change #418
Conversation
ffmpeg/encoder.c
Outdated
| pkt->pts = (int64_t)pkt->opaque; // already in filter timebase | ||
| pkt->dts = pkt->pts - av_rescale_q(pts_dts, encoder->time_base, time_base); | ||
| } | ||
| //fprintf(stderr, "JOSH - flushing %"PRId64"/%"PRId64"\n", pkt->dts, pkt->pts); |
There was a problem hiding this comment.
probably want to remove these commented out lines
There was a problem hiding this comment.
good catch, thanks - fixed in 4175186
| assert.Equal(t, 181, res.Encoded[0].Frames) // should be 180 ... ts rounding ? | ||
| assert.Equal(t, 360, res.Encoded[1].Frames) | ||
|
|
||
| // TODO test rollover of gop interval during flush |
There was a problem hiding this comment.
I'm not sure what we normally do in this repo for TODOs but you maybe want to raise tickets instead if they're important to come back to so we don't forget.
There was a problem hiding this comment.
this specific note is not so important, it is more noting a gap in the test coverage
the other TODOs sprinkled around these tests relate to discrepancies between cpu/gpu behavior are likely related to the green screen flash which we already have ticket(s) for
j0sh
force-pushed
the
ja/resolution-change-crash
branch
from
4175186 to
679bf9c
Compare
Also add another condition for re-initialization: if the input resolution changes. This triggers the filter graph to re-build and adjust to the new resolution, when CPU encoders are in use.
This handles cases where the packet may contain a frame that triggers a decoder reset - we do not want to cause a reset during the flushing process.
This usually happens with CUVID if the decoder needs to be reset internally for whatever reason, such as a mid-stream resolution change. Also block demuxing until decoder is ready to receive packets again.
j0sh
force-pushed
the
ja/resolution-change-crash
branch
from
679bf9c to
0e6fd2e
Compare
|
Squashed the comments from the PR review, changed the merge target to The CI-driven unit tests will probably fail right now even though they pass locally; see #415 (comment) |
Also adds a bunch of other changes necessary to better support
mid-stream resolution changes.
Unfortunately with CUVID there still seems to be a brief flash of
green (looks to be the length of the decoder's internal frame buffer)
but we can tackle that separately. This PR simply makes the transcoder
Depends on #417
The following commits can all stand alone except the last one (unit tests) depends on all of them. I can make separate PRs if that is preferable for ease of review. Let's also keep these commits separate when merging.
ffmpeg: Flush filters before re-initialization.
Also add another condition for re-initialization: if the
input resolution changes. This triggers the filter graph
to re-build and adjust to the new resolution, when CPU
encoders are in use. Resolves a long-standing TODO.
ffmpeg: Re-init encoder on resolution change.
Necessary to have the encoder actually pick up the new
resolution correctly.
ffmpeg: Reset the flush packet after each keyframe.
This handles cases where the packet may contain a frame
that triggers a decoder reset - we do not want to cause
a reset during the flushing process.
ffmpeg: Handle EAGAIN from decoder and drain
This usually happens with CUVID if the decoder needs to be reset
internally for whatever reason, such as a mid-stream resolution
change. Previously, EAGAIN would loop and cause a crash.
Also block demuxing until decoder is ready to receive packets again.
ffmpeg: Add tests for rotation.