wavpack scanning sometimes misses 'smpl' and 'cue ' metadata

[kwhite-uottawa]

WAVfileParser.cpp reads only the first pack from a wavpack file. The RIFF metadata may actually be split between a RIFF header (0x21) and a RIFF trailer (0x22) and separated by many packs. In that case, it's necessary to scan the entire file to get the 'smpl' or 'cue ' metadata. Some offset calculations are also incorrect.

I attach a patch that works around this issue, and a Demo.organ with a single pipe from Friesach that shows the problem.

The patch uses "goto" to workaround the issue; so is more to demonstrate the problem and keep diffs to a minimum rather than to suggest a final fix!

wavpackexample.zip

[larspalo]

@kwhite-uottawa Thanks for reporting and suggesting the changes! I've tried to implement a fix for the issue in commit 13a95e5cc54695bf57ca4df20ed3c04fe294e659 (kind of based on your patch). However, I'd really appreciate it if you in the future would care to make a formal PR with a fully fixed and nicely written code so that you also can stand as contributor of the code here on GitHub and be added in the AUTHORS file.

As it is now, I've re-written the code using your suggestions, of course in my own "manner" and after quite some research in the WavPack documentation, but it will seem that it's done completely by me when in fact I (as well all other users of GoOdf) actually will be indebted to your help! Recognition might not be important to you, but I feel that credit should go to whom it belongs. Anyway, thanks for your help with this!

[larspalo]

@kwhite-uottawa Even if the current code works as far as I've been able to test it, I have a nagging feeling that the break on line 519

    } else {
        // if the RIFF header or trailer contains any other chunks we just ignore them
        break;
    }

potentially could cause a buggy behaviour if for instance a cue, smpl or list info chunk somehow would come after any other non-recognized chunk in the same sub-block. I'd expect it to be better to check the chunk size and jump it, but it seems that the reported chunk size might sometimes very well be larger than the sub-block size (for instance in the case of a found "data" chunk) so it cannot be blindly trusted but must be compared to the sub-block size first. Thoughts on that?

[kwhite-uottawa]

Yes I agree, to be more robust, you should jump over unrecognized chunks by skipping over the minimum of the chunk size and the remaining sub-block size.

I'll keep in mind that you'd prefer PRs in future. I'm neither github nor c++ proficient though.

[kwhite-uottawa]

This b64 encoded wavpack file shows the buggy behaviour you expected. It included a 'fact' and 'PEAK' chunk before 'cue ' or 'smpl'

begin-base64 644 SilentLoop.wav
d3Zwa0QBAAAQBAAAgLsAAAAAAADgLgAAoxgAFf8khL1oAndhdgAhelJJRkbs3AUAV0FWRWZtdCAQ
AAAAAwACAIC7AAAA3AUACAAgAGZhY3QEAAAAgLsAAExJU1REAAAASU5GT0lTRlQkAAAATG9vcEF1
ZGl0aW9uZWVyIChsaWJzbmRmaWxlLTEuMC4yOCkASUNSRAwAAAAyMDIzLTAxLTE2AABQRUFLGAAA
AAEAAAB2v8VjAAAAAAAAAAAAAAAAAAAAAGN1ZSAEAAAAAAAAAHNtcGw8AAAAAAAAAAAAAABhUQAA
AAAAAAAAAAAAAAAAAAAAAAEAAAAAAAAAAAAAAAAAAACIEwAAQJwAAAAAAAAAAAAAZGF0YQDcBQAC
AAMABAAFBgAAAAAAAAAAAAAAAAgCAAAAfyUCAAACASoAigIAAP9/wN0vAuExWhh3dnBrQgAAABAE
AACAuwAA4C4AAOAuAACjGAAV/ySEvQIAAwAEAAUGAAAAAAAAAAAAAAAACAIAAAB/KgCKAgAA/3/A
3S8C07NCeXd2cGtCAAAAEAQAAIC7AADAXQAA4C4AAKMYABX/JIS9AgADAAQABQYAAAAAAAAAAAAA
AAAIAgAAAH8qAIoCAAD/f8DdLwJzTFGAd3Zwa0IAAAAQBAAAgLsAAKCMAADgLgAAoxgAFf8khL0C
AAMABAAFBgAAAAAAAAAAAAAAAAgCAAAAfyoAigIAAP9/wN0vAhPlX4c=
====

Possibly something like this for line 519 (season generously to taste!) :

} else {
  // if the RIFF header or trailer contains any other chunks we just ignore them
  unsigned ubuffer;
  wvFile.Read(&ubuffer, 4);
  if (wvFile.LastRead() == 4) {
    bytesRead += 4;
    totalBytesRead += 4;
    if (subBlockSize - bytesRead >= ubuffer) {
      wvFile.SeekI(ubuffer, wxFromCurrent);
      bytesRead += ubuffer;
      totalBytesRead += ubuffer;
      continue;
      }
    } else {
      return false;
    }
}

[larspalo]

@kwhite-uottawa Yes, exactly. I've committed 1353c46d07bb6258bda61883f3b8ce750c5a5910 that I think should take care of this possibility better, could you please test it?

Of course, a file might be (intentionally or not) so malformed that issues still could arise but I think that any valid file should be handled better now. I hope that the file parsing is designed so that any failure at least would be fairly gracefully handled...

[kwhite-uottawa]

Tested, and confirmed working.

[larspalo]

Tested, and confirmed working.

Thanks for all your help! If nothing new related to this issue is discovered, I'll close this issue when the next release will be done.