TMRh20 / TMRpcm

Arduino library for asynchronous playback of PCM/WAV files direct from SD card. Arduino Uno,Nano,Mega etc supported
http://tmrh20.blogspot.com
592 stars 178 forks source link

HANDLE_TAGS unable to handle trailing _PMX XML metadata tag #201

Open XenonofArcticus opened 7 months ago

XenonofArcticus commented 7 months ago

Adobe Audition adds an XML metadata _PMX chunk after the WAVE data chunk, and the TMRpcm library plays it as PCM data.

The HANDLE_TAGS only seems to handle preceding tags.

I don't have a fix to submit.

Sample attached.

5.zip

TMRh20 commented 7 months ago

Per the Wiki its not supported:

Functions have been added to read Song, Artist, and Album info from ID3v2.3 and LIST tags in WAV files

Note: Audacity is the only supported program to add, modify or edit metadata.

XenonofArcticus commented 6 months ago

If I create a patch for this, would it be welcomed?

TMRh20 commented 6 months ago

Hmm, I guess things to consider are:

  1. How useful this will be. Not sure how many people use the metadata functionality currently
  2. Supporting proprietary software, how many users would use it
  3. How it is implemented, it should use defines like the current tag support

All in all I think it might be worth doing, its just a matter of how you want to implement it.

Also please consider updating the documentation if updating functionality as well, it often gets overlooked.

XenonofArcticus commented 6 months ago

It's not so much "using" the metadata that I'm suggesting (because it's useless junk data), it's that it's nearly impossible to prevent Adobe Audition from adding it, and it wrecks the playback. Any other legit chunk following "data" would also break.

I think the WAVE data loader is actually in error -- the data subchunk of the WAVE chunk in RIFF has its own data size counter, and as best as I can tell, the library isn't utilizing that to load the audio data block, instead it's just loading from the data subchunk header until the end of the file. The _PMX is actually a different legit chunk in the structure and the sizes of the subchunks account for it, in order to make it possible to ignore/skip it. The library just needs to read the data subchunk size properly in order to not overrun on the read (and therefore playback).

In theory, any legit chunk following data would also break the library, not just _PMX.

I think the fix is just that the code around line 370 (and 372 and 377 https://github.com/TMRh20/TMRpcm/blob/master/TMRpcm.cpp#L370 ) need to compute dataEnd using the data subchunk size properly. I've been trying to understand how the data size is actually computed in order to contribute a pull request, but I don't understand it yet.

I don't believe any defines or documentation changes are needed, because it's actually just a functionality fix. I don't see any interest in parsing or utilizing the XML metadata.

On Mon, Feb 19, 2024 at 8:29 AM TMRh20 @.***> wrote:

Hmm, I guess things to consider are:

  1. How useful this will be. Not sure how many people use the metadata functionality currently
  2. Supporting proprietary software, how many users would use it
  3. How it is implemented, it should use defines like the current tag support

All in all I think it might be worth doing, its just a matter of how you want to implement it.

— Reply to this email directly, view it on GitHub https://github.com/TMRh20/TMRpcm/issues/201#issuecomment-1952695119, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPFE3BRODOGL73N3I5QXNTYUNVV3AVCNFSM6AAAAABDOVCZQ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJSGY4TKMJRHE . You are receiving this because you authored the thread.Message ID: @.***>

TMRh20 commented 6 months ago

This I think too https://github.com/TMRh20/TMRpcm/blob/master/TMRpcm.cpp#L626C9-L626C43

The data size isn't really calculated. That code is kind of a mess, but I think that dataEnd should be == to buffSize. I think the lib just needs to grab the data size + header size and use the file position to figure out the end of the file, instead of using available. http://soundfile.sapp.org/doc/WaveFormat/