Fix frame dropping in filtergraph at EOF #153
Conversation
j0sh
force-pushed
the
ja/dropframes
branch
2 times, most recently
from
c428171 to
d722afc
Compare
j0sh
changed the title
There was a problem hiding this comment.
This looks fine to me. However, although unrelated, consider the audio popping issue - I believe that the same affect achieved by this PR would be achieved by the pre-pending and post-pending of one audio frame from the neighboring segments. It's just something to keep in mind.
ffmpeg/ffmpeg_test.go
Outdated
| // to timestamp rounding around EOF. Note this does not happen with | ||
| // mpegts formatted output! | ||
| if res2.Decoded.Frames != 60 || res.Encoded[0].Frames != 61 { | ||
| t.Error("Did not get expected frame counts: is this fixed?!? ", |
There was a problem hiding this comment.
do we need the "is this fixed?!?" bit?
There was a problem hiding this comment.
Happy to make adjustments to the language if you can think of a better phrasing :) Mostly, the intent was to express some notion of:
- This behavior is currently broken, and this unit test serves as reproducible documentation (along with Number of encoded frames does not match number of decoded frames #155)
- But we need to make broken tests "pass"
- If the broken test no longer passes, maybe it means the underlying issue has been fixed? 🤞
- If the test breaks, hopefully a distinct error message will prompt more careful review of the issue and any associated changes.
There was a problem hiding this comment.
ok, that makes a lot more sense now - thank you for explaining :-)
There was a problem hiding this comment.
Updated the language a bit in ab98e51
This PR doesn't attempt to address the audio popping issue; it only fixes frame dropping from the filtergraph as described in #154 . The audio issue (along with your proposed fix) is still open and documented in #152 . |
|
@j0sh can't find way to comment on code that is not changed, so asking here: if (!filter || !filter->active) {
// No filter in between decoder and encoder, so use input frame directly
ret = encode(encoder, inf, octx, ost);
if (ret < 0) return ret;
}shouldn't it exit unconditionally, like this: if (!filter || !filter->active) {
// No filter in between decoder and encoder, so use input frame directly
ret = encode(encoder, inf, octx, ost);
return ret;
} |
@darkdarkdragon Ah yes indeed! Fixed in ab98e51 (Note that we don't actually hit this code path in LPMS itself but I've used it in the past for testing the C API directly...) |
This is done by setting the EOF timestamp of the filtergraph such that the timestamp is *after* the last frame inserted. Also add tests exhibiting various cases uncovered in the process of fixing this.
ffmpeg: Fix frame dropping in filtergraph at EOF.
This is done by setting the EOF timestamp of the filtergraph
such that the timestamp is after the last frame inserted.
Also add tests exhibiting various cases uncovered in the process
of fixing this.
Fixes #154