Fix mp3_parser to not raise error when a tag frame has an unsupported encoding #161
Conversation
|
I think it's fair to treat it as I'm curious how would separate editors deal with such tags. |
|
Looping in @krists as he is the author of the ID3 parsing gem we are using as a dependency. What do you think about this one? Could there be a more generic way of recovering from encoding errors in ID3 parsing? |
|
to give more information, this piece of code is trying to fetch the ENCODING_MAP with key 173: From: /Users/fabioperrella/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/id3tag-0.12.1/lib/id3tag/frames/v2/text_frame.rb:29 ID3Tag::Frames::V2::TextFrame#source_encoding:
27: def source_encoding
28: require 'pry-byebug'; binding.pry
=> 29: ENCODING_MAP.fetch(get_encoding_byte) { raise UnsupportedTextEncoding }.to_s
30: end
[1] pry(#<ID3Tag::Frames::V2::TextFrame>)> get_encoding_byte
=> 173And there is no encoding like that: ENCODING_MAP = {
0b0 => Encoding::ISO8859_1,
0b1 => Encoding::UTF_16,
0b10 => Encoding::UTF_16BE,
0b11 => Encoding::UTF_8
} |
|
Hi! I would not mind adding a configuration option in the parser where you could choose how to handle this situation - raise error or use fallback encoding. In later case there might be issues that the text might have some unexpected characters. But it is probabbly better than thrown exception. Anyone would like to create a PR? |
|
Hi @krists ! I can create a PR for that, tks! |
|
I managed to create an mp3 file and edit it's bytes to simulate an unsupported encoding, like the example that I got from production! I used the app "Tag Editor Free" to edit the V2 tags and SublimeText to edit the binary file and change the encoding. |
fabioperrella
requested review from
julik,
linkyndy,
nitika080289 and
martijnvermaat
fabioperrella
changed the title
lib/parsers/mp3_parser.rb
Outdated
| @@ -1,6 +1,11 @@ | |||
| require 'ks' | |||
| require 'id3tag' | |||
|
|
|||
| ID3Tag.configuration do |c| | |||
There was a problem hiding this comment.
Would it make sense to save the ID3Tag.configuration and restore it once the parser returns? It might be that the application configures ID3Tag differently than we do 🤔
There was a problem hiding this comment.
humm good point! I think it's possible. I will do it next week!
There was a problem hiding this comment.
@julik could you take a look if it is ok now, pls? I used ID3Tag.local_configuration for it!
When working on #161 we noticed that the method `TagWrapper#to_h` does not ignore empty strings and because of that, it can replace a id3v1tag by a empty id3v2 tag, so added a condition to skip for empty values. Example before this change: ``` v1tag.genre = 'Rock' v2tag.genre = '' result: "" ``` Example after this change: ``` v1tag.genre = 'Rock' v2tag.genre = '' result: "Rock" ``` The mp3 used as a fixture was created by us. We used the tool `id3ed` (brew install id3ed) to edit the id3v1 tags
closes #160
I found a file which raises an error when calling the method
tag.album(in id3v2), so I added a loop to call the member methods in the tag insideattempt_id3_v2_extraction. If some of them raise an error, it will consider the tag as invalid and return nil.