Add support for Wave files with LIST section #11
Conversation
src/SDWaveFile.cpp
Outdated
| uint32_t size = 0, data; | ||
| uint8_t buffer[] = {0, 0, 0, 0}; | ||
|
|
||
| fileSize = _file.size() - 36; |
There was a problem hiding this comment.
Could we use sizeof(some struct) here instead of the 36?
There was a problem hiding this comment.
@Rocketct any feedback on this one?
There was a problem hiding this comment.
yes, the 36 byte in this case represent the dimension of the not extended subchunk1 + the dimension of the first three filed of the structure WaveFileHeader, this means that for get this value by sizeof is required or add or subtract the sizeof of different part of this last, for example somthing like sizeof(WaveFileHeader) - sizeof(WaveFileHeader.subchunk2)
Rocketct
force-pushed
the
master
branch
2 times, most recently
from
e69a435 to
485f983
Compare
There was a problem hiding this comment.
@Rocketct looking better, please see the new comments I added.
| _file.close(); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
I think we need the check for header.subChunk1.size != 16 here, like I mentioned below.
There was a problem hiding this comment.
ok after this piece of code make sense, i'll add
src/SDWaveFile.cpp
Outdated
| return; | ||
| } | ||
| //if findData goes fine, data label is assigned to subChunk2.id | ||
| header.subChunk2.id = 0x64617461; |
There was a problem hiding this comment.
This seems strange to assign this here, we can just remove the check below instead?
There was a problem hiding this comment.
i think that id should be assigned for structure integrity i can try the new and the old file if all goes
fine, yes
src/SDWaveFile.cpp
Outdated
|
|
||
| //check if "data" field is present | ||
| while (count < fileSize) { | ||
| if (_file.read((void *)&byteread, 1) != 1) { |
There was a problem hiding this comment.
Maybe consider using:
int b = 0;
/// ...
b = file.read();
if (b == -1) {
///...
}
here.
src/SDWaveFile.cpp
Outdated
| fileSize = _file.size() - offset; | ||
|
|
||
| //check if "data" field is present | ||
| while (count < fileSize) { |
There was a problem hiding this comment.
Another option is to use: while (_file.available()) here, then we don't need the file size calculated and the offset passed in.
There was a problem hiding this comment.
ok is better save space and remove a variable, i'm agree with you
src/SDWaveFile.cpp
Outdated
| int SDWaveFile::read(void* buffer, size_t size) | ||
| { | ||
| uint32_t position = _file.position(); | ||
| int read = _file.read(buffer, size); | ||
|
|
||
| if (position == 0) { | ||
| // replace the header with 0's | ||
| memset(buffer, 0x00, sizeof(struct WaveFileHeader)); | ||
| memset(buffer, 0x00, size); |
There was a problem hiding this comment.
Instead of size here, I think it should be: sizeof(struct WaveFileHeader) + _headerOffset
There was a problem hiding this comment.
mmmm maybe is better remove completely now because is no longer used all the structure in one time but as a single read to the file with different size each time, what do you think?
There was a problem hiding this comment.
We need it, otherwise there will be some "noise" when the wave file is played because the head is interpreted as audio.
There was a problem hiding this comment.
ah ok ok i have understand i just looked better, well this could be done simply with sizeof(struct WaveFileHeader) about _headerOffset, in my opinion is not required because i cut off the fmt's extras byte and send the header as if it is the PCM one, then this https://github.com/arduino-libraries/ArduinoSound/pull/11/files/485f98353ae43feb80a92947e50e3e099005f770#diff-76c1dfd43efb327c34a33ded368fb3c0R210 is done only in the first call of the read (in findDataHeader), let me know if ok eventually i think a way to integrete _headerOffset part in the structure
src/SDWaveFile.cpp
Outdated
| @@ -173,14 +173,48 @@ int SDWaveFile::begin() | |||
| return 1; | |||
| } | |||
|
|
|||
| int SDWaveFile::findData(int offset) { | |||
There was a problem hiding this comment.
Maybe it's clearer if this is renamed to findDataHeaderOffset?
src/SDWaveFile.cpp
Outdated
| return; | ||
| } | ||
|
|
||
| _headerOffset = findData(headerSize); |
There was a problem hiding this comment.
this var could be renamed to something like _dataHeaderOffset.
There was a problem hiding this comment.
yes also in this case is ok
Rocketct
force-pushed
the
master
branch
9 times, most recently
from
ca6743c to
7eede68
Compare
Added, in readheader internal API, a logic to ignore the extrabyte in fmt header's chunk in WAV file.
sandeepmistry
changed the title
Added, in readHeader() internal API, a logic to ignore the extrabytes in fmt header's chunk in WAV file.
I have test the changes with two f.wav files generated from ffmpeg at a sample rate of 22050 Hz and seems that works.
cc/ @sandeepmistry , @agdl if you can make other test
Resolve #9