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

WAV loop and cue positions are not byte offsets #267

Open
d-musique opened this issue May 30, 2024 · 8 comments · May be fixed by #270
Open

WAV loop and cue positions are not byte offsets #267

d-musique opened this issue May 30, 2024 · 8 comments · May be fixed by #270

Comments

@d-musique
Copy link

Hi. The dr_wav library describes certain of fields from WAV metadata as being byte offsets.

Precisely, the fields

  • smpl firstSampleByteOffset and lastSampleByteOffset
  • cue sampleByteOffset

However this does not seem to reflect the reality, because the unit appears to be frames.
(despite what some dubious sources of documentation claim)

This is validated by LoopAuditioneer software.
The source code of openMPT and grandOrgue seem to corroborate. (maybe Polyphone also, I haven't checked)

If a test set is wanted, the Kalvträsk Church file contains wavs with loop and cue.
https://familjenpalo.se/vpo/download/

@rasky
Copy link

rasky commented Jan 1, 2025

"Official" spec form Microsoft also says that it is in samples:
https://www-mmsp.ece.mcgill.ca/Documents/AudioFormats/WAVE/Docs/RIFFNEW.pdf

Also this:
https://www.recordingblogs.com/wiki/sample-chunk-of-a-wave-file

Wavosaur also creates WAVs where the loop points are in samples not bytes.

@d-musique
Copy link
Author

"Official" spec form Microsoft also says that it is in samples:
https://www-mmsp.ece.mcgill.ca/Documents/AudioFormats/WAVE/Docs/RIFFNEW.pdf

Thanks. I had no awareness of the existence of this document.
Indeed it appears to confirm the idea.

@mackron
Copy link
Owner

mackron commented Jan 3, 2025

Thanks. That code was contributed by someone in the community. I'll study the code and get this addressed when I get around to doing another pass on dr_libs stuff. If anyone submits a PR in meantime I'll get it merged.

@rasky
Copy link

rasky commented Jan 3, 2025

I can submit the PR. Can I just rename the fields and so break the API or am I supposed to do something more refined?

@mackron
Copy link
Owner

mackron commented Jan 3, 2025

That's a good question. Since the existing naming says "ByteOffset" we'll have to just break the API and call it "SampleOffset". I'll just bump the version from 0.13.x to 0.14. If you do a PR, do it against the dev branch.

@rasky
Copy link

rasky commented Jan 4, 2025

One obvious option is to keep ByteOffset, add a SampleOffset, and make both fields contain the respective correct value.

This would make existing code work but silently change how the way WAV files. Not sure if it’s better or worse. For instance if somebody realized the problem and use the field ByteOffset as a sample offset, the change would silently break them, even though it is a bug fix.

Maybe it’s better to be loud here, considering that the fix is easy anyway

@mackron
Copy link
Owner

mackron commented Jan 4, 2025

Yep, I support that idea. Lets go with that approach.

@d-musique
Copy link
Author

This would make existing code work but silently change how the way WAV files.

It would break my present code, which depends on ByteOffset actually referring to frames.
Another person who is in the same situation as me might have to deal with an unhappy surprise.
So I'd rather have the API break instead.

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 a pull request may close this issue.

3 participants