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 Theora video issues #101958

Merged
merged 4 commits into from
Feb 17, 2025
Merged

Fix Theora video issues #101958

merged 4 commits into from
Feb 17, 2025

Conversation

berarma
Copy link
Contributor

@berarma berarma commented Jan 23, 2025

This fixes many (if not all) of the long-standing issues when playing Theora video streams. Even though Theora is an old codec, it can still do a decent job, specially when it works 100% correctly. That's what this PR aims to do.

Videos are an essential piece for many games, and I think Godot can be a lot better by fulfilling that need in the core. Having at least a backup codec which can be used out of the box and offers compatibility with old and new hardware will make life easier for a lot of developers.

What's fixed:

  • Duplicate frames being ignored (TH_DUPFRAME). These are used when there are continuous identical frames in the stream.
  • Frames playing ahead of time, more noticeable with lower framerates. The latter can happen when the encoder skips identical frames, but also when the video frame rate is lower than the screen frame rate. Edit: this was made worse by a bug in FFmpeg.
  • Audio buffer running out of data when the previous situations happened and also when decoding was slow.
  • Buffering of the stream didn't work. Now it works for the decoded audio and the encoded video data.
  • The audio buffer couldn't be filled without overwriting previous audio data.
  • Frames weren't being cropped to the original picture size and offset and produced artifacts around the image for some frame sizes.

I've also removed the Theora thread streaming code since it was disabled in code and it would involve more testing. I think it's better to remove that code for now than leave it untested and focus on getting it working 100% right.

The issues fixed could be mostly worked around by setting the keyframe inverval to 1 but this would take away most compression benefits. Now videos should play correctly even at the highest keyframe interval.

And possibly other issues.

There's some code left to dynamically change post-processing levels depending on codec performance, but it doesn't really do anything as it's set to no post-processing. I think I could easily make it work but it would be good to have a setting so the user can disable post-processing or set a max level that suits the video content.

Audio delay compensation is supposed to need some work also according to code comments. I could look at it too.

Please test it. Now that I've gotten a grasp of the code, I'm determined to make it work 100%.

I think this could be easily ported to Godot 3.X.

@DeeJayLSP
Copy link
Contributor

DeeJayLSP commented Jan 23, 2025

Testing videos from #21568 and #66331, the issue seems to be gone with no regressions.

The patch in AudioRBResampler didn't seem to interfere in custom VideoStream implementations, so LGTM.

@berarma
Copy link
Contributor Author

berarma commented Jan 23, 2025

The patch in AudioRBResampler didn't seem to interfere in custom VideoStream implementations, so LGTM.

It will prevent overwriting buffered audio.

@berarma
Copy link
Contributor Author

berarma commented Jan 23, 2025

I've added a fix for #62752. Updated PR description.

@fire fire requested a review from a team January 23, 2025 23:21
@fire
Copy link
Member

fire commented Jan 23, 2025

I'm not able to review this due to time, but I believe that if the test cases work for many people this should be merged for 4.4

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it causes a significant performance regression in debug builds when reading a high-bitrate 1080p60 video despite using a high-end CPU.

PC specifications
  • CPU: Intel Core i9-13900K
  • GPU: NVIDIA GeForce RTX 4090
  • RAM: 64 GB (2×32 GB DDR5-5800 C30)
  • SSD: Solidigm P44 Pro 2 TB
  • OS: Linux (Fedora 41)

See these 120 FPS videos of the running project which should showcase frame drops clearly:

Before

theora_before.mp4

After

theora_after.mp4

Source video file: https://0x0.st/8Xbm.ogv

Thankfully, this is not reproducible when using an optimized editor build (production=yes lto=full), but I worry about low-end and mobile CPUs here.

@fire
Copy link
Member

fire commented Jan 23, 2025

Note that a debug builds is not a release build of Godot Engine. So this may still be ok if we can test on a low end computer or android tablet.

@DeeJayLSP
Copy link
Contributor

Kinda off-topic, but I should warn that Theora doesn't have ARM SIMD enabled in Godot.

I made an attempt a long time ago on #68694, but it added a dependency (perl) and only worked for 32-bit ARM. I presume it's similar to libvpx's buildsystem issue.

@berarma
Copy link
Contributor Author

berarma commented Jan 24, 2025

Tested locally, it causes a significant performance regression in debug builds when reading a high-bitrate 1080p60 video despite using a high-end CPU.

I don't think this is really a performance regression. The code is doing the same work but only in a different way.

I think it's the way I choose the frames to render causing frame stuttering.

Thankfully, this is not reproducible when using an optimized editor build (production=yes lto=full), but I worry about low-end and mobile CPUs here.

I don't think it's directly related to the performance of the code but to the resulting framerate being more or less in sync with the video rate. Thus, it might happen either with low or high end machines.

Running a high-bitrate HD video at 60fps on an unoptimized codec without hardware acceleration is the worst possible scenario. Since the codec is running fully in the CPU, code optimization will be a must in most systems. Anyway, I'm glad you tried this because I think it uncovered an issue in the frame dropping/choosing logic.

I've pushed a new commit with an improved logic that worked well in my tests. I've left printf in the code so that it outputs debug information to the terminal. In case it's still not fixed I'd need that information to see what's happening on your computer. I'll remove them when we're done testing it.

@berarma berarma force-pushed the theora_fixes branch 3 times, most recently from 18048ea to 764f8d3 Compare January 24, 2025 13:17
@DeeJayLSP
Copy link
Contributor

DeeJayLSP commented Jan 27, 2025

Tested the current changes with 5 different videos.

The runtime crashed with two of them right at the end.

Here are the first 4 lines of the backtrace:

[1] /lib64/libc.so.6(+0x1a090) [0x7aa235226090] (??:0)
[2] /home/user/sources/godot-drwav-rebase/bin/godot.linuxbsd.editor.dev.x86_64() [0x5982cf0] (/home/user/sources/godot-drwav-rebase/./thirdparty/misc/yuv2rgb.h:946)
[3] VideoStreamPlaybackTheora::video_write(th_img_plane*) (/home/user/sources/godot-drwav-rebase/modules/theora/video_stream_theora.cpp:77)
[4] VideoStreamPlaybackTheora::update(double) (/home/user/sources/godot-drwav-rebase/modules/theora/video_stream_theora.cpp:472)

@berarma
Copy link
Contributor Author

berarma commented Jan 27, 2025

Tested the current changes with 5 different videos.

The runtime crashed with two of them right at the end.

Thanks for testing. I've tested a bunch of videos and it didn't crash for me but it entered an infinite loop in some of them. I've added conditions for the cases where the streams are incomplete and libogg doesn't detect the end of the stream.

@berarma
Copy link
Contributor Author

berarma commented Jan 27, 2025

I've tried playing some intentionally corrupted files (zeros or random data written in the middle and the end). In all cases the playing continued or stopped depending on the severity of the corruption but it never crashed or became unresponsive, so I think #53503 is also fixed.

@berarma
Copy link
Contributor Author

berarma commented Jan 27, 2025

I've just learned that the frame timestamps calculated by the codec signal the end of frame, not the start. I shouldn't have trusted the original implementation based on the example player. Sorry for the many changes. This makes the code a lot simpler so it's something good after all.

Just for the record, extracting a fragment from an OGG video with the command ffmpeg -ss 00:00:00 -t 00:00:20 -c:v copy -an produces a bad stream, it will play but sequences of identical frames will get mangled. Doing the same operation from another container/codec stream works well. I may file an issue for ffmpeg.

@DeeJayLSP
Copy link
Contributor

DeeJayLSP commented Jan 28, 2025

Tested again

In cases where the video starts with identical frames (the one mentioned in #66331) it seems to skip to the first non-duplicate frame, then freeze until its time is reached.

If the video is played with loop enabled, audio will desync a lot.

@berarma berarma requested a review from a team as a code owner January 28, 2025 17:14
@berarma
Copy link
Contributor Author

berarma commented Jan 28, 2025

Tested again

In cases where the video starts with identical frames (the one mentioned in #66331) it seems to skip to the first non-duplicate frame, then freeze until its time is reached.

I can't reproduce this. I only see a black frame for the first few seconds.

If the video is played with loop enabled, audio will desync a lot.

Fixed. VideoStreamPlayer wasn't flushing the audio buffer between loop plays.

@DeeJayLSP
Copy link
Contributor

DeeJayLSP commented Jan 28, 2025

I can't reproduce this. I only see a black frame for the first few seconds.

For reference, the video was encoded with a keyframe interval of 10 seconds (-g:v 300, the video is 30 FPS)

You should be able to see a small detail from the first non-black frame:
image

This issue was there since way before this PR. It seems to freeze until the video is synced with the audio.

@DeeJayLSP
Copy link
Contributor

DeeJayLSP commented Jan 29, 2025

Sorry, I copied the wrong command. We're using the same options, I don't understand why it works differently. Can you share the video?

This is the one encoded with the reference encoder: https://0x0.st/88Qu.ogv

And this is the one encoded directly with FFmpeg: https://0x0.st/88QS.ogv

You can see the detail from the first frame for a while in the first video, while both freeze for a moment when "valve" is starting to appear.

@berarma
Copy link
Contributor Author

berarma commented Jan 29, 2025

Sorry, I copied the wrong command. We're using the same options, I don't understand why it works differently. Can you share the video?

This is the one encoded with the reference encoder: https://0x0.st/88Qu.ogv

I'm pretty confident this video is encoded like that. So there's an error either in the encoding process or when reading from the source.

This video plays well when the timestamps given by libtheora are interpreted as the start of the frame, but they signal the end of the frame. At first, I used them like this and this video may have played without the freezes but that was wrong. The frozen frame is a normal frame encoded in the stream.

And this is the one encoded directly with FFmpeg: https://0x0.st/88QS.ogv

This one plays fine.

You can see the detail from the first frame for a while in the first video, while both freeze for a moment when "valve" is starting to appear.

I only see the freezing in the first video, and the reason is the same as above. The second one is smooth all the way until the end.

I've observed VLC doesn't play videos correctly. Although it tends to unfreeze frames, maybe because it's using the end of frame as the start, it's incorrect. It can seem like it's doing a good job by unfreezing those frames but it's playing it incorrectly.

ffplay and mpv seem to be reliable, with the only problem that they finish the video before time when there are identical frames at the end without audio (one of my test videos).

@DeeJayLSP
Copy link
Contributor

DeeJayLSP commented Jan 29, 2025

This one plays fine.

I swear I could see the freeze before, but after playing it again, it's gone.

Also, my copy of VLC seems to play both videos correctly with no freezes at all, so idk.

@berarma
Copy link
Contributor Author

berarma commented Jan 29, 2025

This one plays fine.

I swear I could see the freeze before, but after playing it again, it's gone.

Also, my copy of VLC seems to play the first one correctly, so idk.

Yes, VLC plays it without the freeze for me too. That's why I say it doesn't work correctly. Try it with ffplay and mpv and you'll see they show the freezes. They're not really freezes, it's encoded like that.

@DeeJayLSP
Copy link
Contributor

DeeJayLSP commented Jan 30, 2025

Yes, VLC plays it without the freeze for me too. That's why I say it doesn't work correctly. Try it with ffplay and mpv and you'll see they show the freezes. They're not really freezes, it's encoded like that.

On my end:

First video:

  • VLC: Plays normally
  • mpv: Starts playing from the first non-black frame, then freezes for a moment to sync with audio
  • ffplay: Renders the first frame, then begins moving once it's time arrives. Freezes for a moment at the valve logo to sync with audio (same behavior in Godot with this PR in the current state)

Second video:

  • All players work normally.

Well, it's a shame that the encoder that yields the best quality per bytes has this bug. And won't get fixed anytime soon since Xiph.org is likely trying to kill Theora (no commits since 2020).

@berarma
Copy link
Contributor Author

berarma commented Jan 30, 2025

On my end:

First video:

* VLC: Plays normally

* mpv: Starts playing from the first non-black frame, then freezes for a moment to sync with audio

* ffplay: Renders the first frame, then begins moving once it's time arrives. Freezes for a moment at the valve logo to sync with audio (same behavior in Godot with this PR in the current state)

Same as you, but I want to make this perfectly clear: the video with the "freezes" that mpv/ffplay plays is the actually encoded content. So VLC is playing it wrong indeed.

I don't know what you mean when you say it freezes to sync with audio. Audio is playing fine for me all the way through. I honestly think there's nothing wrong with the way mpv, ffplay or this PR play this video.

Oddly enough, I've taken a look at your script and constructed a loosely similar command to convert the file using the Theora example encoder installed in my system:

ffmpeg -i valve.bik -an -c:v rawvideo -pix_fmt yuv420p -f rawvideo - | ffmpeg -s 1920x1080 -f rawvideo -i - -pix_fmt yuv420p -f yuv4mpegpipe - | theora_encoder_example -z 0 -k 300 - -o valve.ogv

And the resulting video plays fine.

Well, it's a shame that the encoder that yields the best quality per bytes has this bug. And won't get fixed anytime soon since Xiph.org is likely trying to kill Theora (no commits since 2020).

The videos that you sent, both have the same video bitrate, and the one encoded with ffmpeg is the smallest one. The audio stream is the same size on both. So ffmpeg seems to do a better job packaging the video stream. I haven't noticed differences in the quality, at least for this one.

@berarma
Copy link
Contributor Author

berarma commented Jan 30, 2025

Just one more discovery about the encoding. ffmpeg uses a default max keyframe interval of 64 , so values higher than 64 are treated as 64. But for some reason, values higher than 64 increase the file size a bit. It's probably generating extra empty packets that are discarded when decoding. Theoretically, the encoder allows values up to 65535 if instructed to do so, but ffmpeg just doesn't. This isn't an issue for this PR since it's all handled by the decoder. Edit: ffmpeg allows for keyframe interval values higher than 64 and they're apparently correctly encoded but timestamps will be wrong in the resulting container file.

Back on topic, I think this PR is as good as it gets. At least, it should be a notable improvement over the current implementation. I'll be testing and following any testing by others but I think it's good to go.

I'm finishing to implement seek capabilities and seamless loops based on the changes in this PR. That will be in another PR coming soon.

@DeeJayLSP
Copy link
Contributor

And the resulting video plays fine.

Until you combine it with an audio stream using -c:v copy -c:a copy

@berarma
Copy link
Contributor Author

berarma commented Jan 30, 2025

And the resulting video plays fine.

Until you combine it with an audio stream using -c:v copy -c:a copy

There's the problem. There's some bug in ffmpeg that I already mentioned before, it doesn't really copy the stream but breaks it. It does no longer contain the same sequence of frames.

@berarma
Copy link
Contributor Author

berarma commented Feb 5, 2025

I've reported bugs for the FFmpeg issues brought up in this PR:

@DeeJayLSP, I'll let you know when there are updates.

These issues don't change anything for this PR. I'll put warnings with workarounds in the Godot docs until they're fixed.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally using https://commons.wikimedia.org/wiki/File:Big_Buck_Bunny_first_23_seconds_1080p.ogv, it works as expected.

Code looks good to me.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

I did some minor review but I am not able to find time to do the test with video files cycle.

Co-authored-by: K. S. Ernest (iFire) Lee <fire@users.noreply.github.com>
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

I did as much code style review as I can. Did not test.

@Repiteo Repiteo requested a review from adamscott February 14, 2025 13:55
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks like a good cleanup, and solid work and testing on the related bug fixes.

I haven't tested myself but let's give it a spin, it's very likely to be a better implementation than what we had before (and we don't have an active Theora / VideoStream maintainer to fully assess the details).

@akien-mga akien-mga merged commit 032cec5 into godotengine:master Feb 17, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@berarma
Copy link
Contributor Author

berarma commented Feb 20, 2025

The Theora bugs in FFmpeg have been already fixed. Details here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment