Skip to content

Wavreader robustness#21916

Merged
PunkPun merged 1 commit into
OpenRA:bleedfrom
robpvn:wavreader-robustness
Oct 21, 2025
Merged

Wavreader robustness#21916
PunkPun merged 1 commit into
OpenRA:bleedfrom
robpvn:wavreader-robustness

Conversation

@robpvn

@robpvn robpvn commented May 16, 2025

Copy link
Copy Markdown
Contributor

This change makes WavReader correctly read the chunk size of a chunk as an unsigned int (https://en.wikipedia.org/wiki/Resource_Interchange_File_Format#Explanation), and cleans up the inconsistent logic around checking chunk sizes. It also attempts to handle buggy/chopped off Wav files where the reported data chunk size is larger than the actual file.

I ran into this while trying to load a Wav file where all the bits in its chunk size field were 1, meaning it first was reported as -1 and after fixing the reading was reported as uint.MAX_VALUE, much longer than the file actually was. These changes should have no impact on correct Wav files.

@PunkPun PunkPun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Overall LGTM. Could you squash the commits?

@penev92 does this supersede #21844?

This change makes WavReader correctly read the chunk size of a
chunk as an unsigned int, and cleans up the inconsistent logic around
checking chunk sizes. It also attempts to handle buggy/chopped off
Wav files where the reported data chunk size is larger than the actual file.

Also adds Robert Nordan to AUTHORS file.
@robpvn
robpvn force-pushed the wavreader-robustness branch from ee5da3b to 11d8e0c Compare May 19, 2025 20:35
@robpvn

robpvn commented May 19, 2025

Copy link
Copy Markdown
Contributor Author

Commits squashed! I don't think this supersedes #21844 since I didn't change anything to do with compressed audio and the fact chunk is still read to get the uncompressedSize as before.

@anvilvapre anvilvapre left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Untested.

We don't do any checks on the value i.e. if they are negative or out of range. But we already didn't do this here or anywhere else.

@PunkPun PunkPun added this to the Next Release milestone Jun 30, 2025
@Mailaender

Copy link
Copy Markdown
Member

https://www.fileformat.info/format/riff/egff.htm#MICRIFF-DMYID.2 confirms that chunk size is uint. Can you provide a defective file that now plays properly?

@robpvn

robpvn commented Jul 29, 2025

Copy link
Copy Markdown
Contributor Author

So, this is a funny story; I can't provide any defective files because I never saved any of them. :D

I was experimenting with using LLMs to provide E.V.A. responses, which I wrote about here: https://www.robpvn.net/2025/07/05/welcome-back-commander-adding-llms-to-openra/ . As a part of that I changed Sound.cs to separate the loading of files to a stream from the parsing of content and added a public method to play streams directly, so I only ever streamed the wav files directly from OpenAI into OpenRA. I didn't submit that change or any of the other horrendous hacks I did to make the proof of concept work. I've used up all the credits I had at OpenAI and I'm not planning on buying any more, so I can't get one of their files now.

But the change definitely fixed the issue I was having!

@PunkPun
PunkPun merged commit 7024385 into OpenRA:bleed Oct 21, 2025
@PunkPun

PunkPun commented Oct 21, 2025

Copy link
Copy Markdown
Member

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