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 OGG Vorbis infinite error spam with corrupt file. #84723

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Nov 10, 2023

When OGG Vorbis encountered unreadable file, it emitted infinite error spam.
This PR returns the full number of frames in the case of error, to prevent infinite loop.

Fixes #84712

Notes

  • Bug occurs because if you return 0 with error, the calling function enters an infinite loop as it keeps trying to read.
  • Have fixed all 3 error conditions, even though 1 was active in the issue.
  • This same bug may be present on other audio readers.

Discussion

Alternatively we could set an error flag. This PR could potentially spam quite a few errors for each audio tile, but it won't enter an infinite loop. If we used an error flag this could prevent more than one error message.

I'm erring on the side of spamming here for now, unless it causes a problem in the wild, or others prefer the flag approach.

When OGG Vorbis encountered unreadable file, it emitted infinite error spam.
This PR returns the full number of frames in the case of error, to prevent infinite loop.
@lawnjelly lawnjelly marked this pull request as ready for review November 10, 2023 15:52
@lawnjelly lawnjelly requested a review from a team as a code owner November 10, 2023 15:52
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 good!

This same bug may be present on other audio readers.

I checked the other _mix_internal and _mix_* functions for AudioStreamPlaybackMP3, AudioStreamPlaybackMicrophone, and AudioStreamGenerator, I don't see similar problems with returning 0 that would end in infinite loops.

They do return 0 on !active condition, which OggVorbis also does in its _mix_internal, so I assume this is expected. But OggVorbis is the only one with ERR_FAIL conditions as part of its actual mixing.

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Nov 10, 2023
@ellenhp
Copy link
Contributor

ellenhp commented Nov 10, 2023

This makes sense to me I think. Returning zero is fine if playback is not active iirc but otherwise it makes sense to return silence. Honestly I'm really reluctant to review or approve anything that touches the interface between the audio server and the streams because Juan wants to refactor that code and I don't want to do anything that will interfere with those plans. This should be small enough that it won't cause problems in a refactor later but I also thought that about #73162. 😕

@akien-mga akien-mga merged commit e38686f into godotengine:master Nov 10, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@lawnjelly lawnjelly deleted the fix_vorbis_inf_loop branch November 11, 2023 06:11
@DevMimas
Copy link

Hello, yes I can confirm the bug in Godot Engine v4.2.1.stable.mono.official. It also occurs for me. Even if you rename a folder in which the video file .OGG is located, there is a crash when reimporting the file. When I deleted the file from the commit, the programme worked again.

Improvement of the text
Unfortunately, the error in the output is meaningless for laymen. I could only find out indirectly that this is related to the OGG file.
Could you please define the error text more clearly?
godotengine/godot-proposals#8808

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.

Specific OGG file triggers "Error during vorbis synthesis -135" spam leading to crash
4 participants