search

CookiePLMonster / mkpsxiso

ISO disc image maker written specifically for PlayStation homebrew development
GNU General Public License v2.0
15 stars 3 forks source link

Add FLAC CDDA support via miniaudio to mkpsxiso #5

Closed G4Vi closed 2 years ago

G4Vi commented 2 years ago

Same changes as were in https://github.com/Lameguy64/mkpsxiso/pull/17, but adapted to use std::filesystem::path.

I noticed there was a mix of spaces and tabs for code indentation in mkpsxiso/main.cpp, so to improve readability in some editors (github) I ran VS code's auto format (which did tabs). That can be undone or redone as spaces if needed.

CookiePLMonster commented 2 years ago

Thanks! Formatting improvements are welcome, but... can you split them into a separate commit if possible? I know it's potentially a lot to ask for, but if it was a separate commit we could use a blame.ignoreRevsFile feature to filter those out from git blame, and thus retain the repo history better.

As for the PR, I'll look into it after I'm done with my current chunk of work on dumpsxiso. How complex do you think it'd be to add support for extracting audio tracks as FLAC?

G4Vi commented 2 years ago

Extracting audio as flac cross-platform is going to be a little trickier. Unfortunately miniaudio/dr_flac only provides flac decoding, not encoding. It's not too difficult to encode with libflac, but setting up libflac cross-platform can be hard (I think that's why lameguy64 abandoned flac support).

As silly as having 2 flac libraries sounds, miniaudio/dr_flac is faster than libflac for decoding, so even if libflac is added, dr_flac probably shouldn't be removed.

CookiePLMonster commented 2 years ago

I'll have to fix this PR for Windows before merging, but before I do so - do you think miniaudio could be moved to a submodule, like tinyxml2 is?

G4Vi commented 2 years ago

miniaudio is now a submodule. It does make the clone significantly bigger, but consistency is probably more important for the source.

G4Vi commented 2 years ago

This PR only applies to non-DA track CDDA right now as the packing for DA tracks is completely separate. Is that okay?

I'm not sure what's the best way forward with that as for an accurate DA file size, the audio must be decoded to PCM so either we'd need keep decoded samples in ram until the CDDA of the DA track is actually written OR put in a filler value for the DA track size and seek back and set it when finally writing the samples.

CookiePLMonster commented 2 years ago

Is that okay?

It forces a prompt refactor to unify those code paths, so I think so. They should be unified regardless in my opinion.

I'm not sure what's the best way forward with that as for an accurate DA file size

Does miniaudio not allow to count the amount of PCM samples in the FLAC file? This information alone should be enough to determine the end size reliably.

G4Vi commented 2 years ago

Does miniaudio not allow to count the amount of PCM samples in the FLAC file? This information alone should be enough to determine the end size reliably.

If it's okay to assume the FLAC's headers are correct then miniaudio can determine the length of PCM frames without decoding for FLAC, even when resampling. If the input audio is MP3 however it needs to do a full decode to determine the pcm length.

So unifying the CDDA packing here is desired?

CookiePLMonster commented 2 years ago

If it's okay to assume the FLAC's headers are correct then miniaudio can determine the length of PCM frames without decoding for FLAC, even when resampling.

I'd say if the header is wrong then the user has bigger issues than a malformed output binary.

So unifying the CDDA packing here is desired?

That's what I'd preferably see, whether it's done as a part of this PR or not. There's already been plenty of code unified in this fork, I don't see why this could not be done too especially since the logic is nearly identical in both cases.

CookiePLMonster commented 2 years ago

Just left one nitpick. Can you rebase it on latest, so I can then push Windows fixes, if any are needed? Then it should be good to merge I think.

G4Vi commented 2 years ago

I'm struggling trying to rebase the changes. A lot of the work was done on src/main.cpp not src/mkpsxiso/main.cpp. Any recommendations on how I should move forward with it?

CookiePLMonster commented 2 years ago

In that case a merge from master to your branch may be easier - and it'll resolve conflicts just as well as rebasing would.

G4Vi commented 2 years ago

Is this all set now? I didn't think there would be windows specific changes for miniaudio, just for libflac.

CookiePLMonster commented 2 years ago

There is some Windows work that needs to be done here, I'll push my changes to this PR and once we verify Linux still works as expected, I'll squash & merge.

CookiePLMonster commented 2 years ago

@G4Vi I'm trying to push my changes but I don't have permissions to do so - I believe you need to enable "Allow edits from maintainers" on the PR for that.

G4Vi commented 2 years ago

@G4Vi I'm trying to push my changes but I don't have permissions to do so - I believe you need to enable "Allow edits from maintainers" on the PR for that. That's weird

image