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

Break infinite loop in FFMpeg::read_frame() #2576

Merged
merged 1 commit into from May 7, 2020
Merged

Break infinite loop in FFMpeg::read_frame() #2576

merged 1 commit into from May 7, 2020

Conversation

dankamongmen
Copy link
Contributor

@dankamongmen dankamongmen commented May 7, 2020

Description

Codecs which set the CODEC_CAP_DELAY capability led to infinite
loops during reading (see #2571). Once AVERROR_EOF is returned,
no more packets are going to be received, regardless of internal
buffering. break out of the loop in that case.

I've verified that this resolves the livelock following decoding
of the first packet in the Notcurses AVI test case which
illustrated this problem. I've also verified that said AVI
advertises the CODEC_CAP_DELAY attribute, matching the proposed
fix. In addition, I verified that OIIO continues to decode all
my other test cases without trouble. I hand-inspected the media,
and verified that the frame counts were precisely correct.

It is legal to pass NULL/0 into the decoder regardless of
CODEC_CAP_DELAY, and thus this whole conditional can really
be eliminated (I have no mention of it in my FFmpeg wrappers).
I haven't bothered to do that here, but it's a thought for the
future. Closes #2571.

Tests

I haven't added any, but you're welcome to test with

https://github.com/dankamongmen/notcurses/blob/master/data/samoa.avi

This failed with oiiotool -a --dumpdata before, but does not now. Instead, we get all expected frames, followed by successful termination.

Checklist:

  • I have read the contribution guidelines.
  • If this is more extensive than a small change to existing code, I
    have previously submitted a Contributor License Agreement
    (individual, and if there is any way my
    employers might think my programming belongs to them, then also
    corporate).
  • I have updated the documentation, if applicable.
  • I have ensured that the change is tested somewhere in the testsuite
    (adding new test cases if necessary).
  • My code follows the prevailing code style of this project.

This transitively resolves dankamongmen/notcurses#547.

Codecs which set the CODEC_CAP_DELAY capability led to infinite
loops during reading (see #2571). Once AVERROR_EOF is returned,
no more packets are going to be received, regardless of internal
buffering. break out of the loop in that case.

I've verified that this resolves the livelock following decoding
of the first packet in the Notcurses AVI test case which
illustrated this problem. I've also verified that said AVI
advertises the CODEC_CAP_DELAY attribute, matching the proposed
fix. In addition, I verified that OIIO continues to decode all
my other test cases without trouble. I hand-inspected the media,
and verified that the frame counts were precisely correct.

It is legal to pass NULL/0 into the decoder regardless of
CODEC_CAP_DELAY, and thus this whole conditional can really
be eliminated (I have no mention of it in my FFmpeg wrappers).
I haven't bothered to do that here, but it's a thought for the
future. Closes #2571.
Copy link
Collaborator

@lgritz lgritz left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for tracking this down. Definitely something that would have taken me a long, long time with my lack of familiarity with ffmpeg API.

@lgritz lgritz merged commit 64de894 into AcademySoftwareFoundation:master May 7, 2020
lgritz pushed a commit to lgritz/OpenImageIO that referenced this pull request May 7, 2020
…n#2576)

Codecs which set the CODEC_CAP_DELAY capability led to infinite
loops during reading (see AcademySoftwareFoundation#2571). Once AVERROR_EOF is returned,
no more packets are going to be received, regardless of internal
buffering. break out of the loop in that case.

I've verified that this resolves the livelock following decoding
of the first packet in the Notcurses AVI test case which
illustrated this problem. I've also verified that said AVI
advertises the CODEC_CAP_DELAY attribute, matching the proposed
fix. In addition, I verified that OIIO continues to decode all
my other test cases without trouble. I hand-inspected the media,
and verified that the frame counts were precisely correct.

It is legal to pass NULL/0 into the decoder regardless of
CODEC_CAP_DELAY, and thus this whole conditional can really
be eliminated (I have no mention of it in my FFmpeg wrappers).
I haven't bothered to do that here, but it's a thought for the
future. Closes AcademySoftwareFoundation#2571.
@dankamongmen
Copy link
Contributor Author

I've got your back, homie =]. Since you're part of my critical infrastructure on at least Fedora, I'm hoping to do a deep dive through src/ffmpeg.imageio sometime this month doing something of an audit. FFmpeg has some definite gotchas. I also hope to improve video decoding performance, which (as I alluded to in an earlier bug) is kinda surprisingly bad, no offense. Using my FFmpeg backend directly, I'm able to decode frames quickly enough that I need carefully wait for frame timings to move on; using OIIO, the videos I'm testing with require ~10x realtime to decode, with sustained 100% CPU burn. I'll get on top of it for ya. =]

@lgritz
Copy link
Collaborator

lgritz commented May 7, 2020

That would be great. It was contributed several years back with the main aim being just being able to pull individual frames out, not with speed. It has received little attention since then -- just the usual touch-ups for new ffmpeg releases, new compiler releases with new warnings, that kind of thing. Definitely needs a good scrubbing.

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.

[BUG] oiiotool + my app hit infinite 0-byte read() loop on certain AVIs OIIO video playback locks up
2 participants