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

Audio decomp, supersedes #1236 #1509

Draft
wants to merge 161 commits into
base: main
Choose a base branch
from
Draft

Conversation

playerskel
Copy link
Contributor

This PR supersedes #1236 by @MNGoldenEagle. I have tried to continue working on @MNGoldenEagle (and team)'s great work where they currently lack the time to finalize this PR for merging.

Some things covered here (in no particular order):

  • removes whitespaces and weird characters in (auto-generated) filenames
  • get rid of the ELF creation way of embedding data into the final ROM
  • automatically generate the audio_init_params.c constants upon music (.aifc) changes
  • apply some, but certainly not all, feedback from @Dragorn421 and @engineer124
  • merged with the latest master branch

Not sure if it's ideal, but instead of creating ELF files I have now opted to generate C files directly from within the audio-related Python scripts. These are then compiled into object files etc. This can be improved further, but it works fairly well for now.

I'd be happy to apply other feedback or necessary changes, but I tried to steer away from making bigger picture decisions such as naming schemes etc that were mentioned in #1236. Will reply in #1236 to clarify what has been applied in this PR and what has not. Looking forward to feedback.

MNGoldenEagle and others added 30 commits October 9, 2021 15:50
{"local":
  {"subdir":  "tools/seq64"
  ,"action":  "clone"}
,"remote":
  {"url":     "https://github.com/sauraen/seq64"
  ,"branch":  "master"
  ,"commit":  "b2c5f8348"}
,"git-subrepo":
  {"version": "0.1.0"
  ,"commit":  "???"
  ,"origin":  "???"}}
{"local":
  {"subdir":  "tools/seq64"
  ,"action":  "clone"}
,"remote":
  {"url":     "https://github.com/sauraen/seq64"
  ,"branch":  "master"
  ,"commit":  "b2c5f8348"}
,"git-subrepo":
  {"version": "0.1.0"
  ,"commit":  "???"
  ,"origin":  "???"}}
…bankToC

{"local":
  {"subdir":  "tools/AudiobankToC"
  ,"action":  "clone"}
,"remote":
  {"url":     "git@github.com:sauraen/AudiobankToC.git"
  ,"branch":  "main"
  ,"commit":  "2dc7f2617"}
,"git-subrepo":
  {"version": "0.1.0"
  ,"commit":  "???"
  ,"origin":  "???"}}
@ariahiro64
Copy link
Contributor

2023-04-02.12-30-48.mp4

after messing around i was able to replace voice clips

@playerskel
Copy link
Contributor Author

playerskel commented May 18, 2023

@engineer124 @fig02 @Dragorn421 hey team, was wondering if there are concrete things we/I can work on to get this merged or if it's not yet entirely clear to the rest of the team (which I could understand too). There's always things to improve further, but the audio system works rather well and it should be a fairly straightforward merge. So kind of wondering what can be done to get this going before the PR becomes stale and outdated etc. Thanks.

@RevoSucks
Copy link
Contributor

Is there any update on this?

@Dragorn421
Copy link
Collaborator

A "todo list" for this PR (very high level and fragmentary)

I'm partly rewording comments from others, this isn't all from me. I hope I'm not including outdated information
Corrections welcome!

Build system

  1. Samples can (and should) be decompressed on extraction, and matching compression exists. However this needs additional information on how to compress the file, which isn't provided by the build system at the moment

  2. It does not use parallel jobs where it could, making it needlessly slow

  3. The binaries are basically assembled from arrays of bytes the python scripts spit out, where we could instead make use of assembler syntax (gas (gnu as)) to produce legible output. It would also be easier to maintain and follow

Data

  1. It's unclear what we can/should commit to the repo and what should be extracted. As it stands, some files being extracted means modifying the audio assets even to simply add things is not git friendly. Committing a few files would help

  2. Some data is extracted too raw and would be a lot more useful with more processing (e.g. "drums and tuning values", I'm told)

Documentation

  1. The new tools written for this PR are very undocumented (both in workings and usage)

  2. The documentation provided by the PR is incomplete, one file simply stops in the middle of a sentence (the one on sequence language iirc)

  3. It would be better to document the audio code fully prior to handling the assets, to have a better (and provably correct) understanding of How Things Work, as well as agreed-upon names. In particular the sequence language interpreter

@Thar0
Copy link
Contributor

Thar0 commented Jun 13, 2023

I've done some work already to address some of the things above, such as the "drums and tuning values" issues and reworking the build system. If anyone would like to collaborate on those feel free to reach out.

We also need feedback from anyone who has already tried to use this and what their experiences were. What worked as expected? What caused problems? If you have used this, please comment or bring it up in the discord. It would be very helpful for determining further improvements.

@HailToDodongo
Copy link

Few things i noticed while testing this:

  • just compiling doesn't pick up any changes in .seq files, i have to run "setup" every time before compiling
  • because setup ran, it will compile all sequences instead of only the ones i changed
  • I also have a temp-file problem, every compile after a "setup" run creates exactly 4030 empty files in /tmp/
    • this also happens for non-audio changes, just with fewer files, the main repo does not have this problem

For the setup i made a smaller task just for the audio:

setup_audio:
	python3 tools/audio/assemble_sequences.py assets/sequences build/include build
	python3 tools/audio/assemble_sound.py assets/soundfonts build/assets build/include assets/samples --build-bank --match=ocarina -q

For the temp files, no idea where they are from, but i was able to see a few with lsof before the process stopped:

/tmp  lsof -r1 ctm*
COMMAND    PID    USER   FD   TYPE DEVICE SIZE/OFF   NODE NAME
cfe     153941 mbeboek    1w   REG   0,35        0 162047 ctmf8xxGLE
cfe     153941 mbeboek    3w   REG   0,35        0 162047 ctmf8xxGLE

But i did not find any reference to "cfe" in the repo.

I'm testing directly on linux (arch).

@hensldm
Copy link
Contributor

hensldm commented Jul 23, 2023

Few things i noticed while testing this:

* I also have a temp-file problem, every compile after a "setup" run creates exactly 4030 empty files in /tmp/
  
  * this also happens for non-audio changes, just with fewer files, the main repo does not have this problem

...

For the temp files, no idea where they are from, but i was able to see a few with lsof before the process stopped:

/tmp  lsof -r1 ctm*
COMMAND    PID    USER   FD   TYPE DEVICE SIZE/OFF   NODE NAME
cfe     153941 mbeboek    1w   REG   0,35        0 162047 ctmf8xxGLE
cfe     153941 mbeboek    3w   REG   0,35        0 162047 ctmf8xxGLE

But i did not find any reference to "cfe" in the repo.

I'm testing directly on linux (arch).

This is due to outdated ido recomp binaries. The older version had a bug that would leave empty temp files around.

@HailToDodongo
Copy link

Few things i noticed while testing this:

* I also have a temp-file problem, every compile after a "setup" run creates exactly 4030 empty files in /tmp/
  
  * this also happens for non-audio changes, just with fewer files, the main repo does not have this problem

...
For the temp files, no idea where they are from, but i was able to see a few with lsof before the process stopped:

/tmp  lsof -r1 ctm*
COMMAND    PID    USER   FD   TYPE DEVICE SIZE/OFF   NODE NAME
cfe     153941 mbeboek    1w   REG   0,35        0 162047 ctmf8xxGLE
cfe     153941 mbeboek    3w   REG   0,35        0 162047 ctmf8xxGLE

But i did not find any reference to "cfe" in the repo.
I'm testing directly on linux (arch).

This is due to outdated ido recomp binaries. The older version had a bug that would leave empty temp files around.

Ah thanks! just merged the latest master in, and the temp files are no longer created.

@fig02 fig02 added the Needs discussion There is a debate or other required discussion preventing changes from being made label Aug 27, 2023
@Dragorn421 Dragorn421 marked this pull request as draft April 19, 2024 12:25
@Dragorn421
Copy link
Collaborator

This is going to be superseded by a newer audio extraction tooling effort
cf the discord for details

@Dragorn421 Dragorn421 removed the Needs discussion There is a debate or other required discussion preventing changes from being made label Apr 19, 2024
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.

None yet