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

Replace stb_vorbis with libvorbis #52406

Merged
merged 1 commit into from
Sep 10, 2021
Merged

Conversation

ellenhp
Copy link
Contributor

@ellenhp ellenhp commented Sep 5, 2021

Fixes a variety of crashing bugs in stb_vorbis, and I think it also fixes #26964 but I will have to do a bit more testing to confirm that.

This introduces a size penalty of approximately 5% over the old internal representation of ogg files, but I think getting rid of stb_vorbis is worth it.

Important: If this gets merged and we ever want to revert it or switch to a new library or something, it's critical that we keep the change I made to editor_file_system.cpp otherwise whatever .ogg importer we replace this one with won't be run, and the engine will just crash instead.

@ellenhp ellenhp force-pushed the libvorbis branch 5 times, most recently from 96292e7 to 1d4d1f5 Compare September 5, 2021 02:50
@aaronfranke aaronfranke added this to the 4.0 milestone Sep 6, 2021
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.

You need to update the documentation for the CI checks to pass.

@reduz
Copy link
Member

reduz commented Sep 6, 2021

This is looking extremely good so far!

@ellenhp
Copy link
Contributor Author

ellenhp commented Sep 6, 2021

I switched the names around, moved OGGPacketSequence to its own file in the ogg module, switched to using cache-friendlier data structures for the ogg packet data, and generally cleaned things up a bit. What this PR really needs right now is testing, not necessarily an in-depth review. Happy to respond to comments but what I'm most concerned about right now is it not actually working with some ogg files due to something I missed in the spec. It works with anything created with ffmpeg from my experience.

I'll put this in the PR description too but storing the ogg bitstream as a list of packets in a variant array does result in a size penalty compared to storing it unprocessed. The penalty is fairly severe before compression (25%+), but seems to be about 2-4% after re-compressing the saved oggvorbisstr file in a zip. I assume that during exports there will be compression applied so I'm hopeful this isn't a big issue?

Additionally, this PR splits out the OGGPacketSequence from the vorbis code. I'd prefer to keep it this way and to expose the OGGPacketSequence code because that will make an opus plugin very easy to write and maintain.

edit: There are architectural issues with reusing the OGGPacketSequence code, so it might need to be duplicated in an opus plugin. Still, keeping it separate from the vorbis code makes logical sense to me.

@ellenhp ellenhp force-pushed the libvorbis branch 3 times, most recently from 42424ab to 4b9f078 Compare September 7, 2021 04:32
@ellenhp
Copy link
Contributor Author

ellenhp commented Sep 7, 2021

I think (hope?) I've addressed all the CI issues. This should be ready to review now.

@ellenhp ellenhp marked this pull request as ready for review September 7, 2021 04:46
@ellenhp ellenhp requested review from a team as code owners September 7, 2021 04:46
@Calinou
Copy link
Member

Calinou commented Sep 7, 2021

I assume that during exports there will be compression applied so I'm hopeful this isn't a big issue

PCK files don't use compression to speed up random file access, but ZIP archives exported by Godot do. Nonetheless, the vast majority of users use PCK files.

@ellenhp
Copy link
Contributor Author

ellenhp commented Sep 7, 2021

Nonetheless, the vast majority of users use PCK files.

Good to know. Let's wait to merge this until I figure out how to reduce the size penalty. I think I have some ideas.

@ellenhp
Copy link
Contributor Author

ellenhp commented Sep 8, 2021

The size penalty for the new imported type (oggvorbisstr) now seems to be around 5% over the old imported type (oggstr), which is pretty good.

I'm discarding some information to accomplish this then reconstructing it:

  • b_o_s (beginning of stream): I can reconstruct this perfectly because each OGGPacketSequence contains only one logical stream, so this is 1 for the first page, 0 otherwise.
  • e_o_s (end of stream): Similarly to b_o_s, I'm reconstructing this by looking at the position in the stream of any given page.
  • granule_pos: I store this unmodified. I need it for seeking so I can't discard it.
  • packetno: I reconstruct this by starting at zero for the first packet popped out of the OGGPacketSequence, then incrementing it for each packet. I reset it to zero whenever a seek is performed. This works, but it's not compliant with the standard I'm sure. I went through the libvorbis source code and it's only ever used for detecting holes/missing packets in the stream as far as I can tell, so I think what I'm doing is fine. The value isn't important, all that matters is that it increments by 1 for each contiguous packet.

There's one additional caveat to this PR:

  • The importer and stream code currently only support ogg vorbis files with a single logical stream, so "chained" streams will only partially import. All logical streams after the first will be discarded. I don't know how common these files are in practice, but I suspect they're very uncommon.

@reduz
Copy link
Member

reduz commented Sep 9, 2021

Looks really good! Please squash commits so I can review and eventually merge.

@reduz
Copy link
Member

reduz commented Sep 10, 2021

This looks amazing, awesome job!

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.

Godot Editor crash when re-importing .ogg song that is currently playing
4 participants