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

Refactor OGG export and always use VBR #7697

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

sakertooth
Copy link
Contributor

@sakertooth sakertooth commented Feb 9, 2025

A user reported that there were facing issues playing back exported OGG files on certain applications. FFmpeg was used as a source of truth to reproduce the bug, as it was able to detect such packet decoding errors quite quickly. Applying these fixes seems to have resolved those errors.

Edit: I eventually realized that errors were still being outputted by FFmpeg, so I just went to town and refactored everything. However, I made certain changes, like always using VBR, that may or may not be wanted. This is because after I did my refactoring in AudioFileOgg, using vorbis_encode_init_vbr seemed like the recommended approach. Also, from the libvorbisenc documentation:

Although the Vorbis codec is natively VBR, libvorbis includes infrastructure for 'managing' the bitrate of streams by setting minimum and maximum usage constraints, as well as functionality for nudging a stream toward a desired average value. These features should only be used when there is a requirement to limit bitrate in some way. Although the difference is usually slight, managed bitrate modes will always produce output inferior to VBR (given equal bitrate usage). Setting overly or impossibly tight bitrate management requirements can affect output quality dramatically for the worse.

Given that, I think we can safely stick to using VBR without problems. If anyone has objections though, I'd be curious to know.

@sakertooth sakertooth marked this pull request as draft February 9, 2025 14:47
@sakertooth sakertooth marked this pull request as ready for review February 9, 2025 15:21
@sakertooth
Copy link
Contributor Author

Thanks for the review @PhysSong. The code looks good to me now, so let me know if there's anything else or you approve 👍

@sakertooth sakertooth changed the title Fix packet decoder errors in exported OGG files Refactor OGG export and always use VBR Feb 14, 2025
@tresf
Copy link
Member

tresf commented Feb 18, 2025

I think we can safely stick to using VBR without problems. If anyone has objections though, I'd be curious to know.

With regards to variable bitrate (VBR), here's a 2008 article at Audacity pretty much stating the same thing, quoting:

[...] encoding to a target bitrate is now officially deprecated for all uses except streaming over bandwidth-critical connections.

@tresf
Copy link
Member

tresf commented Feb 18, 2025

Thanks for the review @PhysSong. The code looks good to me now, so let me know if there's anything else or you approve 👍

From what I can tell, the code has changed considerably since @PhysSong left a review. I believe a re-review is warranted. The title and PR description have changed considerable as well.

@tresf tresf requested a review from PhysSong February 18, 2025 04:29
@tresf
Copy link
Member

tresf commented Feb 18, 2025

Tested on macOS using PortAudio and SDL, at various bitrates, no observable issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants