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

Parse <sample> with base64 data #1041

Merged
merged 1 commit into from
Nov 24, 2021
Merged

Conversation

paulfd
Copy link
Member

@paulfd paulfd commented Nov 21, 2021

Also added the necessary plumbing to load from RAM.
It's missing AIFF, and metadata reading since those API are heavily file-based for now.
Note that this is not exactly what's proposed in #91, since the goal is also to allow to "copy" these embedded samples as printable characters (e.g. in an edit box on the web, or a pastebin).

The syntax is as follows:

<region> sample=test.wav
<sample> name=test.wav encoding=base64 data=[... Base64 data ...]

Samples like this will be loaded fully in RAM; I made some modifications to the file pool to check both "fully loaded" and preloaded files on requests, which wasn't the case before.
The encoding=base64 is required to try and keep a compatibility path with the Rapture encoding mentioned in #91 down the line.
When reading an opcode value, the parser now behaves differently if the opcode name is data: it will ignore comments since base64 has the / sign, and will only stop on EOF, a new < character, or a new # character.
The underlying base64 decoder ignores whitespace, and can support optional padding with =.

Now that I think about it and write it down, it's not going to be compatible as is with #91 because the parser for the latter stops at a different character ($). Maybe a different opcode name (e.g. base64data= and data=) to allow the parser to parse these items differently? @jpcima, your thoughts?

Missing or would-be-nice, possibly for further PRs:

  • Changes depending on the above discussion.
  • Rewrite the metadata reader to allow to read from memory instead of just files. The way to proceed is probably to have a new provision in the abstract AudioReader to read the raw data directly, regardless of the source. This part was not obvious when I refactored this so for the time being only basic metadata will be read mostly for WAV files. Once done, the path parameter added to the AudioReader constructors is unneeded.
  • Expand the AIFF library with a memory version.
  • Read gzipped/bzipped data, which would reduce a bit the size of the base64 fields.

@paulfd paulfd requested a review from jpcima November 22, 2021 10:23
@paulfd paulfd marked this pull request as ready for review November 22, 2021 10:23
Also added the necessary plumbing to load from RAM
It's missing AIFF since the API is file-based for now.
@paulfd
Copy link
Member Author

paulfd commented Nov 24, 2021

I changed it to read the base64data field in the end, and updated the metadata reader, as well as adding a bunch of tests. Should be all good.

@paulfd paulfd merged commit f8ffb52 into sfztools:develop Nov 24, 2021
@paulfd paulfd mentioned this pull request Nov 24, 2021
@paulfd paulfd deleted the sample-headers branch January 21, 2022 10:28
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.

1 participant