Godot: Webm playback broke after 3.2 update [regression from af9bb0ea]

[gdesecrate]

Godot version:
3.2 Steam

OS/device including version:
Linux
Issue description:
Video playback does not works.

Steps to reproduce:
Make VideoPlayer node, give it webm file as stream, play it.

Minimal reproduction project:
VideoPlayer node with webm file

[Calinou]

Please upload a minimal reproduction project to make this easier to troubleshoot.

[alexfreyre]

I have the same issue. Video playback does not works.

[gdesecrate]

Here is project
playback failure on Godot 3.2.zip

While making this project I found that:

1. Webm does not play if it does not have audio track (I use webms with no audio for menu backgrounds)

1. If audio track is present - the playback drops TON of frames and really slow and unpleasant to watch

[alexfreyre]

I am working on a VR project, where I need to use videos without audio and for a spatial effect the audio aside.

[Sslaxx]

Can't reproduce here - Godot 3.2, Windows 10. This may be an issue with Linux only (possibly macOS)?

[alex9099]

Can reproduce on linux with godot

2\. If audio track is present - the playback drops TON of frames and really slow and unpleasant to watch

I can't reproduce the second topic, it seems to play without a problem for me (maybe a little bit slower, not sure if that would be placebo), but only if it has audio. it worked fine on 3.1

[Sslaxx]

Does the terminal give any errors/warnings playing the webm file(s)?

[alex9099]

I don't see any (just the warning about delta not being used), I just rechecked and indeed the video with sound plays a little bit slower and the one without sound doesn't play at all on 3.2

[alex9099]

This regression was introduced in 12cc760538fbec37199dcc24098d942ac1d623b8 . Seems like it worked fine in b1f5cee7d9a1f509ef8990f3b8405c74e83a20cc

No idea how to fix the problem though, but it was introduced there

[akien-mga]

CC @20kdc

[gdesecrate]

If I ffmpeg the audio track to the video playback is like 4fps.
Really ruins the video playback.
https://www.youtube.com/watch?v=XDUdjJb5p9M

I checked if ogv works - sample file seems worked fine.

Now I have to convert all videos to ogv... =(

[20kdc]

If b1f5cee (my commits) worked fine, but 12cc760 (the merging into Godot) failed, then the cause is somewhere between c7ba1e2 (the parent of my commits) and 12cc760 (when my commits got merged), or there were modifications involved.
So I guess first check for any on-merge modifications...
Then, on a testing branch, rebase everything up to b5deb1d (the commit before the merge) onto b1f5cee (to add my commits to the scenario)... then bisect?

[20kdc]

Ok, so git diff c7ba1e2 b5deb1d | less is informative enough that I think that range of commits should be tested for the audio stream regression.
(Obviously, ignoring any colour issues that result from not having my patches.)

[20kdc]

Just confirmed. Here (EDIT: Linux X11), b5deb1d acts the same as master (EDIT: i.e. the bug is present)

[20kdc]

Well, I've now determined that due to linking RAM requirements, bisecting would be terrible for me.
But just to clarify that previous comment:

alex9099 says the regression was introduced in 12cc760 (the merge commit for my commits), but also says b1f5cee (the end of my commits) worked fine.
b5deb1d precedes 12cc760 on the master line, so the bug being on b5deb1d indicates that it appeared after I forked and before the merge.
Hence the range b7c50d9...b5deb1d (commit after fork base to commit before 12cc760) contains the regression.

EDIT: Commits Tested So Far AFAIK to simplify bisects:
12cc760 (master) tested by alex9099, result: bad
b1f5cee (fork based on c7ba1e2) tested by alex9099, result: good
b5deb1d (master) tested by me (20kdc), result: bad
c7ba1e2 (master) untested, but if builds take a long time, assume good due to b1f5cee

[akien-mga]

Thanks everyone for the debugging!

I finished the bisect started by @20kdc and identified the commit that introduced the regression: af9bb0ea15dfd3dfe8950fcfcce364485dadd92a

Note that it doesn't compile out of the box since the previous commit e5ed112d69fb0a4118b8b40de4d3851cac845445 introduced a build failure, which was only fixed in the next commit 040b59c010f3cce63b4c45956c418b74079e24e6.

It can be fixed with this local diff for testing:

diff --git a/scene/audio/audio_stream_player.cpp b/scene/audio/audio_stream_player.cpp
index 865f672def..310ce468f3 100644
--- a/scene/audio/audio_stream_player.cpp
+++ b/scene/audio/audio_stream_player.cpp
@@ -92,7 +92,7 @@ void AudioStreamPlayer::_mix_internal(bool p_fadeout) {
 void AudioStreamPlayer::_mix_audio() {

    if (!stream_playback.is_valid() || !active ||
-           (stream_paused && !stream_fade)) {
+           (stream_paused && !stream_paused_fade)) {
        return;
    }

[alexfreyre]

I found after many tests, that in 3.1.2 plays the video faster, but in 3.1.1 much faster

being able to play a video webm, 4k, with constant bitrate of 20 000 k, without audio, "decently"

completely correct in FULL HD, even creating a videoplayer as the parent of a meshintance, the material (0) calling the current frame of the video as texture for: albedo, roughness, metallic, and at the same time.

in 3.1.1 perfectly
in 3.1.2 slow and with lags
in 3.2 impossible in webm without sound (although a web with sound shows you 1 random frame at random times too)

I don't know if the dialogue fits here and I don't know what is used for play video inside Godot, but personally
 I find that at least in Linux the gstreamer library is incredibly fast, I have also tried how ffplay from ffmpeg plays videos with any kind of postprocessing, in real time and extremely fast,

I hope it improves the use of video within godot, because it is a very useful tool to use as dynamic textures in 3D meshes or environments, not only for cutscenes or similar uses.

[akien-mga]

This hack works it around:

diff --git a/modules/theora/video_stream_theora.cpp b/modules/theora/video_stream_theora.cpp
index cf1fc3f175..4f723be792 100644
--- a/modules/theora/video_stream_theora.cpp
+++ b/modules/theora/video_stream_theora.cpp
@@ -364,7 +364,7 @@ void VideoStreamPlaybackTheora::set_file(const String &p_file) {

 float VideoStreamPlaybackTheora::get_time() const {

-   return time - AudioServer::get_singleton()->get_output_latency() - delay_compensation; //-((get_total())/(float)vi.rate);
+   return time - delay_compensation; //- AudioServer::get_singleton()->get_output_latency()
 };

 Ref<Texture> VideoStreamPlaybackTheora::get_texture() const {
diff --git a/modules/webm/video_stream_webm.cpp b/modules/webm/video_stream_webm.cpp
index 41f9e67672..c286ce7231 100644
--- a/modules/webm/video_stream_webm.cpp
+++ b/modules/webm/video_stream_webm.cpp
@@ -394,7 +394,7 @@ int VideoStreamPlaybackWebm::get_mix_rate() const {
 inline bool VideoStreamPlaybackWebm::has_enough_video_frames() const {
    if (video_frames_pos > 0) {

-       const double audio_delay = AudioServer::get_singleton()->get_output_latency();
+       const double audio_delay = 0; //AudioServer::get_singleton()->get_output_latency();
        const double video_time = video_frames[video_frames_pos - 1]->time;
        return video_time >= time + audio_delay + delay_compensation;
    }
@@ -402,7 +402,7 @@ inline bool VideoStreamPlaybackWebm::has_enough_video_frames() const {
 }

 bool VideoStreamPlaybackWebm::should_process(WebMFrame &video_frame) {
-   const double audio_delay = AudioServer::get_singleton()->get_output_latency();
+   const double audio_delay = 0; //AudioServer::get_singleton()->get_output_latency();
    return video_frame.time >= time + audio_delay + delay_compensation;
 }

The issue is that in af9bb0e, @reduz renamed AudioServer::get_output_delay() to AudioServer::get_output_latency(), and fixed the implementation so that it returns the latency calculated by the specific audio driver.

That should be fine, but actually, get_output_delay() used to return 0 in its base implementation, and it was never overridden... so in practice all code that relied on it always got a zero delay value.

So af9bb0e fixed a bug, but it introduced issues in code which was somehow working only thanks to the bug.

If anyone more familiar than me with audio mixing wants to have a look at it, it would likely make sense to review the few calling points for get_output_latency() and ensure that they properly handle a non-zero latency.

[20kdc]

Since it didn't work in the first place, would it be safe to say that outright removing latency compensation would be a valid fix?
EDIT: Specifically removing latency compensation from the WEBM module, not places where it's already working.

[akien-mga]

As mentioned above, that's a hack, but yes, it fixes the regression without introduce any new bug (but we now know that latency compensation likely doesn't work at all either).