Skip to content
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

ffmpeg: Fix short segments flushing without using NULL packets #189

Merged
merged 2 commits into from
May 15, 2020

Conversation

jailuthra
Copy link
Contributor

@jailuthra jailuthra commented May 6, 2020

  • Maintain diff of sent packets/recv frames to know when decoder is done flushing.
  • Stop sending sentinel packets after n=5 attempts of not getting any valid frame back.
  • Fix fps filter dropping 1-frame segments by sending a flush frame with a longer pts.
  • Add parameterized unit tests to reproduce transcoding fails on segments containing very few video frames #168

In #187 I tried to fix the short segment flushing using NULL packets, but it had heavy performance impact. So I've created this fresh PR because that got very messy.

I tried my best to benchmark this against master and it seems to perform equally well. I noob though, pls verify :)

@jailuthra jailuthra requested a review from j0sh May 6, 2020 08:32
Earlier we relied on getting a sentinel (pts -1) packet back to know
when the decoder is flushed. This does not work for short segments as
the decoder buffers packets internally.

Now we maintain the difference between packets sent - frames received
and mark decoder as flushed when this becomes 0 (usually).

Sometimes the decoder might not return all frames (see #155), so we give
up after sending SENTINEL_MAX packets and not getting any valid frame
back.
ffmpeg/ffmpeg_test.go Outdated Show resolved Hide resolved
ffmpeg/ffmpeg_test.go Outdated Show resolved Hide resolved
ffmpeg/ffmpeg_test.go Outdated Show resolved Hide resolved
ffmpeg/lpms_ffmpeg.c Outdated Show resolved Hide resolved
ffmpeg/lpms_ffmpeg.c Outdated Show resolved Hide resolved
ffmpeg/api_test.go Outdated Show resolved Hide resolved
@jailuthra jailuthra force-pushed the jl/flush2 branch 2 times, most recently from 135d352 to 30b566f Compare May 15, 2020 16:17
ffmpeg/api_test.go Outdated Show resolved Hide resolved
if err != nil {
t.Error(err)
}
if res.Encoded[0].Frames == 0 || res.Encoded[1].Frames != 0 || res.Encoded[2].Frames != fc {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Checking for res.Encoded[0].Frames == 0 makes me sweat a bit, because we don't really know what might be a reasonably correct value. But I understand it's pretty hard to get predictable results across different runs, probably OK for now.

- Use expected output pkt pts in flush packet to ensure fps filter
returns a frame even for 1-frame segments getting downsampled to low
fps. FFmpeg CLI drops such frames unless using eof_action=pass, but
we cannot use it as it closes the decoder.

- Add parameterized unit tests to reproduce #168

Fixes #168
Copy link
Collaborator

@j0sh j0sh left a comment

Choose a reason for hiding this comment

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

LGTM! 🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants