Skip to content

Conversation

@DeeJayLSP
Copy link
Contributor

@DeeJayLSP DeeJayLSP commented Sep 28, 2022

As libtheora GIT (2020.10) was mentioned in #52410, I decided to take that update.

The patch folder was removed, as the newer version includes it.

As this PR mostly touches code that is unchanged from 3.x to 4.0, I believe this is cherry pickable.

By running some tests I got the conclusion that libtheora GIT is about 18% faster than 1.1.1.

I tested decoding a 1080p50 video with both 1.1.1 and GIT built from source, both results ran on the same CPU.

  • 1.1.1 managed to decode at 68FPS
  • GIT managed to decode at 81FPS

@DeeJayLSP DeeJayLSP requested review from a team as code owners September 28, 2022 00:23
@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Sep 28, 2022

It seems that some builds break due to cpu.c being replaced with x86/x86cpu.c and x86_vc/x86cpu.c.

It seems some tasks don't have env x86_libtheora_opt_gcc.

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Sep 28, 2022

I tried to build this on Linux (with tools) and it worked just fine, any hints on what's happening with checks? I'm sure it's something to do with the buildsystem.

@aaronfranke
Copy link
Member

thirdparty/libtheora/x86/x86int.h:55:11: fatal error: x86cpu.h: No such file or directory
   55 | # include "x86cpu.h"
      |           ^~~~~~~~~~

The file it's looking for does not exist. Here is the folder: https://github.com/DeeJayLSP/godot/tree/update_theora/thirdparty/libtheora/x86

Note that it does exist in the "x86_vc" folder, but it's missing from the "x86" folder.

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Sep 28, 2022

Weird, it does exist on the folder I'm pushing.

Will try to figure out how to force it.

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Sep 28, 2022

This is being caused by .gitignore, which has a x86/ on it. Gonna force add it.

Squashing commits as soon as macOS check passes.

This can be cherry picked to 3.x if approved.

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Sep 28, 2022

I see that /arm is also missing. Requiring it later or not, I'm going to force it too.

In this current state, Godot crashes on Android when trying to play an .ogv. Maybe it will be required, a change on the buildsystem is needed.

x86/x86cpu.c only gets called by platforms that define x86_libtheora_opt_gcc (Linux, macOS and Windows through mingw)

x86_vc/x86cpu.c only gets called by platforms that define x86_libtheora_opt_vc (Windows through MSVC)

We probably need a way to call it on every possible platform by matching the correct architecture, and probably arm/armcpu.c (and arm/armstate.c) if the architecture is any ARM (defining something like x86_libtheora_opt_arm.

In the previous version there was a cpu.c included by default for all platforms.

@DeeJayLSP DeeJayLSP marked this pull request as draft September 28, 2022 14:07
@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Sep 28, 2022

By testing the very same changes but done in 3.5, I got the conclusion that armcpu.c is not needed at all. The two previous commits can be reverted (keeping the arm folder).

The crash only happens in 4.0. With or without theora update. x86cpu.c is a dependency that isn't compiled outside of Linux/macOS/Windows anyway.

With this in mind, considering that it worked for 3.x on Android, I assume the problem is not being caused by this PR.

Reopened for review.

@DeeJayLSP DeeJayLSP marked this pull request as ready for review September 28, 2022 18:10
@DeeJayLSP
Copy link
Contributor Author

Running the binary from this PR on a phone with Vulkan support worked with no crashes and played the video just fine.

I would like to say this PR is complete.

@Calinou
Copy link
Member

Calinou commented Oct 14, 2022

Out of curiosity, does this dependency update improve decoding quality or reduce CPU usage?

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Oct 14, 2022

All I am 100% sure is that outside platforms that add either x86_libtheora_opt_gcc or x86_libtheora_opt_vc to env, you have one less file compiled in, which is cpu.c.

For x86_libtheora_opt_gcc, which specifies files to build on Linux, macOS and Windows through mingw, it adds the files x86/sse2idct.c and x86/x86cpu.c.

  • By comparing a Linux build with and without these changes, it was possible to see an increase of just 32 bytes.

For x86_libtheora_opt_vc, which specifies files to build on Windows through MSVC, it adds only x86_vc/x86cpu.c.

As I mentioned before, Ogg Theora videos play normally other platforms that don't add any of those.

And according to this 12 year old article there is a slightly higher decoding speed.

I don't think devs at xiph.org are updating theora any soon, as their focus seem to be on AV1 nowadays.

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Oct 15, 2022

With further search (thanks to FNA-XNA/Theorafile, which recently updated to upstream libtheora), I now know that arm/, c64x/, x86/ and x86_vc are optimizations for specific CPU types.

Unfortunately, the ARM optimizations within it (which would be amazing to have here) seem incompatible with Godot's buildsystem, since it isn't purely C/C++ (it includes GNU Assembler and Perl). Also, this optimization seems to apply only for arm32.

I have not found examples of building arm/ other than Theorafile and libtheora itself (it does have a SConstruct file, but it doesn't even touch arm), and I don't understand makefiles.

Update 1: Apparently you have to use the perl script to generate some *-gnu.S files. That extension seems to be supported by SCons.

Update 2: I don't know how to get it to build.

Update 3: libpng does something similar, but GNU Assembler files are already in the source instead of having to be generated with a Perl script.

For now I'm keeping x86 and x86_vc optimizations as both were already in. I'm leaving arm implementation to anyone interested in making the changes for it to work (but only if this PR ever gets merged, because arm depends on this).

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Oct 19, 2022

One last force push just to remove arm/ and c64x/ folders (and point out they have been removed). Both are not being used.

If merged, anyone wishing to implement the arm optimizations is welcome to add the folder back, make it work and open another PR with it. This one just will just keep the current optimizations.

If I learn how to implement arm optimizations in time, I may introduce it here as a second commit. I learned, but I believe it's better to do a second PR since I don't want it to block the library update behind a review with changes requested.

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.

Approved in PR review meeting.

We have slight concerns about using a Git branch from a mostly dead project when their latest stable release was 1.1.1 from 2011... but apparently ffmpeg is using the Git branch for its binary releases and other projects are also using it successfully. Early test results also seem promising. So let's give it a spin and see how it goes.

As mentioned above, ARM optimizations should be added in a follow PR as we do want them for Android, iOS, macOS M1 and various Linux and Windows platforms.

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Nov 15, 2022

As I said in the meeting (and forgot to mention here), the ARM optimizations in libtheora GIT only build for arm32, so like libpng, it can only be built for armv7 Android.

@akien-mga akien-mga merged commit c52d836 into godotengine:master Nov 15, 2022
@akien-mga
Copy link
Member

Thanks!

@DeeJayLSP DeeJayLSP deleted the update_theora branch November 15, 2022 16:19
@DeeJayLSP DeeJayLSP restored the update_theora branch November 15, 2022 16:20
@DeeJayLSP DeeJayLSP deleted the update_theora branch November 15, 2022 17:41
@DeeJayLSP
Copy link
Contributor Author

I forgot to mention: even Firefox uses libtheora GIT.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants