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

Fix AV1/VP9 (and send/receive API use in general) #428

Merged
merged 4 commits into from
Feb 15, 2024

Conversation

dwbuiten
Copy link
Member

This PR does two things:

  • Properly sets the delay when AV1 is being decoded, which is possible since FFmpeg/FFmpeg@1b05d27. We can make this slightly more generic to all codecs, but I want to change that later in a separate change set so as to keep any introduced bugs separate.
  • Properly handles AVERROR(EAGAIN) from avcodec_send_packet by stashing the packet to send again on the next call, and from avcodec_receive_frame by increasing the delay counter.

Since this makes changes to the core decode loop(s), it needs some good testing before merging. Particularily to confiirm AV1 and VP9 are, in fact, fixed (seeking, and linear).

Should (probably) fix #319, and makes #323 obsolete, I think. May or may not fix #352.

CC: @tdaede @jamrial @wangqr

@myrsloik
Copy link
Contributor

Looks reasonable, will try on some random av1 files later

src/core/videosource.cpp Show resolved Hide resolved
src/core/videosource.cpp Show resolved Hide resolved
int Ret = avcodec_send_packet(CodecContext, Packet);
if (Ret == AVERROR(EAGAIN)) {
// Send queue is full, so stash packet to resend on the next call.
DelayCounter--;
Copy link

Choose a reason for hiding this comment

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

If we can delay more than once, shouldn't the stashed packet be a stash queue?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, since we don't read another packet until the stashed one is successfully sent.

src/core/videosource.cpp Outdated Show resolved Hide resolved
@CoffeeFlux
Copy link

Anything I can do to help move this along? I'm looking at getting a new Aegisub stable out but I'd like to have proper VP9/AV1 support, and don't want to build against a fork.

@dwbuiten
Copy link
Member Author

dwbuiten commented Dec 5, 2023

Anything I can do to help move this along? I'm looking at getting a new Aegisub stable out but I'd like to have proper VP9/AV1 support, and don't want to build against a fork.

Mostly it is a function of my time (of which I have little at the moment...) - the current PR seems to have issues, which I need to debug.

@L4cache
Copy link

L4cache commented Dec 14, 2023

How about using libvpx to decode vp9? It seems more multi-thread capable than native one (but maybe slightly slower in single-thread) and with support of alpha channel in addition.

It seems not suffer form #352. Let me list in detail:

  1. ffms2 current HEAD, ffmpeg branch release/6.1, native vp9 decoder: broken
  2. ffms2 current HEAD, ffmpeg branch release/6.1, libvpx decoder: good
  3. ffms2 with quietvoid's patch , ffmpeg branch release/6.1, native vp9 decoder: looks ok with regularly skipped(duplicated) frames
  4. ffms2 with quietvoid's patch , ffmpeg branch release/6.1, libvpx decoder: good
  5. ffms2 tag 2.40, ffmpeg branch release/4.3, native vp9 decoder: broken
  6. ffms2 tag 2.40, ffmpeg branch release/4.3, libvpx decoder: looks ok but frame count and fps are off
  7. This PR, ffmpeg branch release/6.1, native vp9 decoder: good
  8. This PR, ffmpeg branch release/6.1, libvpx decoder: good

All above are built with msys2 mingw-w64 (as used by media-autobuild_suite), decoded data from all "good" results are exactly the same

@L4cache
Copy link

L4cache commented Dec 14, 2023

I encountered this error opening files with video+audio with several combination of codecs after this PR.
vapoursynth.Error: Source: Insanity detected: decoder returned an empty frame

But not all file of problematic combination will cause error, for example vp9+opus produced by ffmpeg fails but some files downloaded from YouTube don't.
If audio is removed then the decoding will be successful.

@dwbuiten
Copy link
Member Author

dwbuiten commented Feb 9, 2024

@L4cache I am revisiting this PR currently, can you provide an example file?

@L4cache
Copy link

L4cache commented Feb 10, 2024

@L4cache I am revisiting this PR currently, can you provide an example file?

https://transfer.sh/85oyj7LBrm/sample.zip

I forgot to mention, the way I apply this pr is https://github.com/L4cache/ffms2/tree/test-3
Many more video+audio codec combinations are affected, like, basically anything but h264 or h265 + commonly used lossy audio encoders (libmp3lame is broken but libshine is ok so I say encoder)

@dwbuiten
Copy link
Member Author

@L4cache Thanks, I was able to reproduce the issue using your sample. I will look into it.

This has been possible since FFmpeg/FFmpeg@1b05d27.

Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
If we get AVERROR(EAGAIN) while trying to send a packet, we are supposed
to try resending it after receiving some more. Same for receiving, but the
other way around.

Very pleasant API to use.

Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
@dwbuiten
Copy link
Member Author

@L4cache I found the bug. I have updated the PR.

Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
@dwbuiten dwbuiten changed the title [NO-MERGE-YET] Fix AV1/VP9 (and send/receive API use in general) Fix AV1/VP9 (and send/receive API use in general) Feb 14, 2024
@dwbuiten dwbuiten merged commit c03dc64 into FFMS:master Feb 15, 2024
0 of 3 checks passed
@dwbuiten dwbuiten deleted the EAGAIN branch February 15, 2024 11:33
@dwbuiten
Copy link
Member Author

Merged, let the testing begin!

Note thay CI failed for unrelated reasons. Tests pass locally, aside from HDR metadata which is being fixed upstream in FFmpeg right now.

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.

VP9 decodes to scrambled video after fefe4b97 Re-Visit VP9
7 participants