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

Webm playback broke after 3.2 update [regression from af9bb0ea] #35760

Closed
gdesecrate opened this issue Jan 30, 2020 · 22 comments · Fixed by #35993
Closed

Webm playback broke after 3.2 update [regression from af9bb0ea] #35760

gdesecrate opened this issue Jan 30, 2020 · 22 comments · Fixed by #35993

Comments

@gdesecrate
Copy link

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
Copy link
Member

Calinou commented Jan 30, 2020

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

@alexfreyre
Copy link

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

@gdesecrate
Copy link
Author

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)

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

@alexfreyre
Copy link

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

@Sslaxx
Copy link

Sslaxx commented Jan 31, 2020

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

@alex9099
Copy link

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
Copy link

Sslaxx commented Jan 31, 2020

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

@alex9099
Copy link

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
Copy link

alex9099 commented Jan 31, 2020

This regression was introduced in 12cc760 . Seems like it worked fine in b1f5cee

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

@akien-mga
Copy link
Member

CC @20kdc

@gdesecrate
Copy link
Author

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
Copy link
Contributor

20kdc commented Feb 1, 2020

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
Copy link
Contributor

20kdc commented Feb 1, 2020

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
Copy link
Contributor

20kdc commented Feb 1, 2020

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

@20kdc
Copy link
Contributor

20kdc commented Feb 1, 2020

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 akien-mga self-assigned this Feb 5, 2020
@akien-mga
Copy link
Member

Thanks everyone for the debugging!

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

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

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;
 	}

@akien-mga akien-mga changed the title Webm playback broke after 3.2 update Webm playback broke after 3.2 update [regression from af9bb0ea] Feb 5, 2020
@alexfreyre
Copy link

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
Copy link
Member

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
Copy link
Contributor

20kdc commented Feb 7, 2020

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
Copy link
Member

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).

akien-mga added a commit to akien-mga/godot that referenced this issue Feb 7, 2020
af9bb0e fixed AudioServer's
`get_output_delay()` (which used to always return 0) while renaming it
to `get_output_latency()`. It now returns the latency from the
AudioDriver, which can be non-0.

While this was a clear bugfix, it broke playback for WebM files without
audio track. It seems like the playback code, even though it queried
the output delay to calculate a time compensation, was designed to work
even though the delay value was actually bogus. Now that it's correct,
it's not working.

As a workaround we comment out uses of the output latency, restoring
the behavior of Godot 3.1.

This code should still be reviewed by someone more versed in video
playback and fixed to properly account for the non-0 driver latency.

Fixes godotengine#35760.
akien-mga added a commit to akien-mga/godot that referenced this issue Feb 14, 2020
af9bb0e fixed AudioServer's
`get_output_delay()` (which used to always return 0) while renaming it
to `get_output_latency()`. It now returns the latency from the
AudioDriver, which can be non-0.

While this was a clear bugfix, it broke playback for WebM files without
audio track. It seems like the playback code, even though it queried
the output delay to calculate a time compensation, was designed to work
even though the delay value was actually bogus. Now that it's correct,
it's not working.

As a workaround we comment out uses of the output latency, restoring
the behavior of Godot 3.1.

This code should still be reviewed by someone more versed in video
playback and fixed to properly account for the non-0 driver latency.

Fixes godotengine#35760.

(cherry picked from commit da411d1)
@Atermnus
Copy link

Atermnus commented Dec 23, 2021

3.4, re-opening this since it lags again.

4K video doesn't play at all without sound, with sound it plays extremely slow, file size is 1.2 MB

Full HD video doesn't play as well, lowest quality supported is around 640x480 where I notice no lag (with sound).

@Calinou
Copy link
Member

Calinou commented Dec 23, 2021

3.4, re-opening this since it lags again.

This is already being tracked in #55815; please continue the discussion there.

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