Skip to content

Conversation

nikitalita
Copy link
Contributor

There are a bunch of music editors love to set invalid Vorbis comments in ogg vorbis files that they export (e.g. Sony's likes to write "Sony Ogg Vorbis 1.0 Final"), and this results in a lot of logspam whenever an ogg like this is loaded up. We should really only be emitting a warning about it if tools are enabled.

@nikitalita nikitalita requested a review from a team as a code owner August 21, 2025 23:19
@AThousandShips AThousandShips added this to the 4.x milestone Aug 22, 2025
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

This would be a good opportunity to link to the specification, explaining why Godot cares about the comment at all.

@nikitalita
Copy link
Contributor Author

https://xiph.org/vorbis/doc/v-comment.html

Basically, comments are structured like env vars in vorbis, VAR=VALUE. This is how tags are stored (artist, album, etc.), and Godot parses them out for display. When it comes across one that is not formatted like this, it throws a warning.

@aaronfranke
Copy link
Member

@nikitalita Can you put that in a comment in the code?

@nikitalita nikitalita force-pushed the invalid-ogg-comment branch from be19b86 to df7c7d8 Compare August 23, 2025 00:21
@nikitalita
Copy link
Contributor Author

done

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

Should be squashed, but this looks good to me.

@nikitalita nikitalita force-pushed the invalid-ogg-comment branch from 3ab7490 to 9aa9bac Compare August 25, 2025 17:55
@nikitalita
Copy link
Contributor Author

I can't re-run the failed jobs

@nikitalita
Copy link
Contributor Author

I think this is ready to go.

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.

3 participants