Compiler compatibility and unicode termination fixes #55
Conversation
Kept all "size" values in frames metadata untouched, but now any strlen()-like functions work correctly. Fixes issue larsbs#54
| // Adding string termination bytes in case the stored string doesn't have those | ||
| for (int i = 0; i < string_termination_bytes; ++i) | ||
| text[text_size + i] = 0x00; |
There was a problem hiding this comment.
Is this due to a malformed id3tag embedded in the mp3? or how can we reach that scenario?
There was a problem hiding this comment.
Also, why do we need a for loop here instead of just adding the string termination bytes? It's only gonna run for two iterations.
There was a problem hiding this comment.
It seems that it's not malformed, but intended - there's no zeroes after the strings in actual mp3 files, I've checked multiple mp3 files I've had. Probably because storing the string + its length is sufficient for them. So we want to put those zeros into the RAM ourselves, if we want to rely on strlen-like functions.
Regarding for loop - I was actually debating between this and two normal assignments, neither looks better than the other to me. Looks like you favor the other way, so I'll switch to that :)
| @@ -37,13 +37,19 @@ ID3v2_TextFrame* TextFrame_parse(CharStream* frame_cs, const int id3_major_versi | |||
| CharStream_seek(frame_cs, ID3v2_FRAME_ENCODING_LENGTH, SEEK_CUR); // skip encoding | |||
|
|
|||
| const int text_size = header->size - ID3v2_FRAME_ENCODING_LENGTH; | |||
| char text[text_size]; | |||
| const size_t string_termination_bytes = 2; | |||
| char* text = malloc((string_termination_bytes + text_size) * sizeof(char)); | |||
There was a problem hiding this comment.
It's been a while since I've worked on this, but, doesn't text_size already include the string_termination_bytes length?
There was a problem hiding this comment.
Based on mp3's I've worked with, there's no termination, so it's not included
| CharStream_read(frame_cs, comment, comment_size); | ||
|
|
||
| ID3v2_CommentFrame* frame = CommentFrame_new(header->flags, lang, short_desc, comment); | ||
|
|
||
| FrameHeader_free(header); // we only needed the header to parse the data | ||
|
|
||
| free(comment); | ||
| free(short_desc); |
There was a problem hiding this comment.
| free(short_desc); | |
| free(short_desc); | |
| ID3v2_TextFrame* frame = TextFrame_new(header->id, header->flags, text); | ||
|
|
||
| FrameHeader_free(header); // we only needed the header to parse the data | ||
|
|
||
| free(text); |
There was a problem hiding this comment.
| free(text); | |
| free(text); | |
There was a problem hiding this comment.
I also think we can group those free's with FrameHeader_free (by deleting line 51) if it makes sense to you
| CharStream_read(frame_cs, pic_data, pic_size); | ||
|
|
||
| ID3v2_ApicFrame* frame = | ||
| ApicFrame_new(header->flags, description, picture_type, mime_type, pic_size, pic_data); | ||
|
|
||
| FrameHeader_free(header); // we only needed the header to parse the data | ||
|
|
||
| free(mime_type); | ||
| free(description); | ||
| free(pic_data); |
There was a problem hiding this comment.
| free(pic_data); | |
| free(pic_data); | |
Oh! Honestly, I haven't noticed there are unit tests inside. I'll run them next time I'm working on that project. |
Fixes issues #48 and #54, see issues' descriptions for details