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

flac seems to write malformed AIFF-C, both none and sowt #767

Open
H2Swine opened this issue Dec 2, 2024 · 5 comments · Fixed by #768
Open

flac seems to write malformed AIFF-C, both none and sowt #767

H2Swine opened this issue Dec 2, 2024 · 5 comments · Fixed by #768

Comments

@H2Swine
Copy link
Contributor

H2Swine commented Dec 2, 2024

In the course of testing input file handling after this one - used: 1.4.3 and including https://github.com/xiph/flac/actions/runs/12089446877 which I think is the most recent?

Running the following on the attached .flac:

flac --force-aiff-c-none-format -dfo aiffcnone.aiff short.flac
flac --force-aiff-c-sowt-format -dfo aiffcsowt.aiff short.flac

... gets me the two .aiff files in the attachment.

Then try flac --no-padding aiffc*.aiff :

WARNING: FORM chunk size of file aiffcnone.aiff does not agree with filesize
aiffcnone.aiff: wrote 2222 bytes, ratio=0,135
WARNING: FORM chunk size of file aiffcsowt.aiff does not agree with filesize
aiffcsowt.aiff: wrote 2222 bytes, ratio=0,135

With --keep-foreign-metadata-if-present added:

WARNING: FORM chunk size of file aiffcnone.aiff does not agree with filesize
aiffcnone.aiff: WARNING reading foreign metadata: invalid AIFF file: unexpected EOF (012)
aiffcnone.aiff: wrote 2306 bytes, ratio=0,140
WARNING: FORM chunk size of file aiffcsowt.aiff does not agree with filesize
aiffcsowt.aiff: WARNING reading foreign metadata: invalid AIFF file: unexpected EOF (012)
aiffcsowt.aiff: wrote 2306 bytes, ratio=0,140

aiffc.zip

@ktmf01
Copy link
Collaborator

ktmf01 commented Dec 3, 2024

I just made a pull request for a potential fix for this issue. Could you try again with the binary under https://github.com/xiph/flac/actions/runs/12135436767 (as soon as it is finished)

@H2Swine
Copy link
Contributor Author

H2Swine commented Dec 3, 2024

It seems to resolve the invalid files creation, but I discovered a few things that may or may not be related. First, how it behaves on those files now:

  • Trying to --keep-foreign-metadata-if-present --force-aiff-format on a .flac with AIFC foreign metadata (none or sowt), throws this error: ERROR verifying foreign metadata restore from withforeignmetadata.aifcsowt.flac to outfile.aiff: stored main chunk length differs from written length Sure it isn't strange that it objects, but maybe it should throw a different one?
  • Similar happens when trying to force AIFC formats on a .flac with AIFF foreign metadata. But if trying to "force none on a sowt or vice versa", then it gives the error that stored foreign format block differs from written block. Perhaps the file is being restored to a different format than that of the original file.

So it looks like the decoder knows that AIFC-none and AIFC-sowt are different, but not that old-fashion AIFF and AIFC are different formats?

Doing the same with --keep-foreign-metadata-if-present --force-[the wave and rf64 formats], reveal the same as in the first item above: ERROR verifying foreign metadata restore from flacfile.flac to outfile.<extension>: stored main chunk length differs from written length. Maybe it should throw the other error.

Then a couple of issues I discovered when testing

  • I see that flac.exe now outputs .aifc, but -o outfile.aifc (sans any foreign metadata preservation) does not try to force AIFF-C?
  • Running --keep-foreign-metadata-if-present on .flac files generated from metadata-less .aiff, gives the following warning: WARNING reading foreign metadata: invalid WAVE file: missing "fmt " chunk (024) . Sure it isn't supposed to have that ...

@H2Swine
Copy link
Contributor Author

H2Swine commented Dec 3, 2024

Also, it might leave suspicious or corrupted decoded files upon throwing those errors, but that is maybe expected.

@ktmf01
Copy link
Collaborator

ktmf01 commented Dec 3, 2024

So it looks like the decoder knows that AIFC-none and AIFC-sowt are different, but not that old-fashion AIFF and AIFC are different formats?

This is 'first come, first served' in case of decoding AIFF to AIFF-C or vice versa, the total length of the file is different and this is encountered first. In the case of mixing up sowt and NONE, the file length is the same, and the first error is a different COMM chunk.

I am not sure changing that is practical, but I can take a look.

Then a couple of issues I discovered when testing

  • I see that flac.exe now outputs .aifc, but -o outfile.aifc (sans any foreign metadata preservation) does not try to force AIFF-C?

Yes, that is correct. I forgot to add that apparently. However, I wonder, in case a filename ends in aifc, should flac default to NONE, sowt or throw an error that this is ambiguous.

  • Running --keep-foreign-metadata-if-present on .flac files generated from metadata-less .aiff, gives the following warning: WARNING reading foreign metadata: invalid WAVE file: missing "fmt " chunk (024) . Sure it isn't supposed to have that ...

I'll take a look, should be an easy fix.

@ktmf01 ktmf01 closed this as completed in dca8eaf Dec 3, 2024
@ktmf01 ktmf01 reopened this Dec 3, 2024
@H2Swine
Copy link
Contributor Author

H2Swine commented Dec 3, 2024

I wonder, in case a filename ends in aifc, should flac default to NONE, sowt or throw an error that this is ambiguous.

My first reaction is, "decoder chooses". Because we have already gotten used to it doing so for -o outfile.wav.
But then, what to choose? On one hand why choose aifc for integer LPCM if not for sowt? On the other hand, why choose sowt on an 8-bit signal except for creating test files?

Watch out or you are in for a --prefer-output-format=[ordered list] ;-)

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.

2 participants