Allowed loading non standard compliant WAV files which lack zero padding of last chunk

[d-musique]

This avoids padding the offset past the length of file.

Otherwise, the check (offset != length) fails, being 1 byte off, and the loader throws an error.

I find a marginal number of files to be affected, but since these are copyrighted, I don't attach them.

aplay describes such a file as: Signed 24 bit Little Endian in 3bytes, Rate 48000 Hz, Stereo

[larspalo]

As a very simple matter of fact, it's the wav file with such a length chunk (odd) that's not conforming to the specs that the real cause of this (it's written with some software that's buggy). The original GO code is correct.

[d-musique]

As a very simple matter of fact, it's the wav file with such a length chunk (odd) that's not conforming to the specs that the real cause of this (it's written with some software that's buggy). The original GO code is correct.

This file does not seem to contain metadata about what software created it. Its organ was made through Hw->ODF conversion, so it was supposed to be compatible with Hauptwerk originally. Regardless of incorrectness, most sound software seem happy to load it.

[larspalo]

I even suspect that this PR could potentially cause issues as the read size of the chunk (in a file that actually conforms to the specification) does not include the padding zero byte. It's definitely something that needs serious testing but the author of the PR has not enabled workflows.

From the wav file spec regarding the chunk size: A 32-bit unsigned value identifying the size of ckData. This size value does not include the size of the ckID or ckSize fields or the pad byte at the end of ckData.

[larspalo]

Regardless of incorrectness, most sound software seem happy to load it.

Yes, many times, depending on where the wrong offset would occur, the file could still be usable. But I suspect that if an earlier chunk (than the last) would be read like this PR does the offset could potentially be wrong for reading the next chunk and that would seriously affect things.

[d-musique]

Yes, many times, depending on where the wrong offset would occur, the file could still be usable. But I suspect that if an earlier chunk (than the last) would be read like this PR does the offset could potentially be wrong for reading the next chunk and that would seriously affect things.

When this code skips incrementing offset, it would be guaranteed to be looking at the final chunk. because length refers to the length of the whole file, not that of the chunk. Afaict, it should not have incidence on other chunks.

I am trying to get the workflows working on my side.

[d-musique]

Yes, but the line just above the if does get executed every time a chunk would be processed and since that now lacks the taking into account of any zero padding... It might work in 99% cases, but can you guarantee that it would work in every?

• The line above the if runs at all times.
• The line below the if runs:
  • guaranteed on "all except last" chunks (by loop invariant)
  • conditional on "last" chunk, depending whether or not file contains a final pad byte, as checked by offset < length

Frankly, it's just a prevention measure to prevent offset from going overboard after we've done reading and it's no longer needed, and just so it doesn't trigger the "Invalid WAV file" error.

But, if you want to go even more permissive without changing a thing within the loop: Another possibility is removing this "Invalid WAV file" check altogether https://github.com/GrandOrgue/grandorgue/blob/8dae62028792bc0aee29afeef943f5335fa4a6d3/src/core/GOWave.cpp#L263-L264

Which is, accepting any file which contain trailing junk / or lack final padding. That would go equally well as I'm concerned.

[larspalo]

@d-musique Yes, looking closer at it, the correct padding offset would indeed be applied up until the very last chunk check. So it seems to be working. However, the title of this PR is wrong and some comment will need to be added in to code.

This is not to "fix" loading, which was working exactly as the wave standard supposes it to earlier. What this PR instead does is to "allow" loading a non standard compliant wav sample file , which should also be the title. A comment in the code above this change stating the acknowledging of allowing non standard compliant wave files should be added to indicate that we know the real issue, just like done earlier in the wave loading when deviations from standard was done.

[d-musique]

Is this fine?

[larspalo]

Is this fine?

I'd prefer having the comment just below the original comment and not on the same line as the if.