Support multiple TXXX ID3 tags #1234
Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
|
Error in CLA occurred because of my global Git configuration on computer I worked, what should I do in this case ? My GitHub account match in CLA but commits are using different email address |
|
CLAs look good, thanks! |
|
Fixed myself |
|
Hi. Please could you provide some test content that requires this change? Thanks! |
|
Sure, we have video that contains information for mid rolls in TXXX frames http://aftonbladetlive-lh.akamaihd.net/i/20150928224817na_1@182509/master.m3u8?custom-mdt=on (don't ommit custom-mdt param) you should be able to see some extra TXXX at the beginning of this video Because college of mine is working on iOS application, I know that extra information in TXXX frames are available. Those extra TXXX frames should contain keys like "duration" or "trigger" and those TXXX frames are there and are parsed correctly by parser but value in ID3 frames Map is constantly overwritten by last TXXX value. |
|
Thanks! Looking at the spec, it appears PRIV and GEOB can both be repeated as well, and for the general case we need to assume repeats are possible too. So I think we need to come up with a representation that supports repeated frames in general. Preferably a lightweight one; I don't really want to end up with each Id3Tag being a Map<String, List>. What about if we make |
|
Right, it's better solution for repeated frames of various types. I will correct this based on your suggestions, is that ok ? |
|
That would be great; thanks! Don't worry about backward compatibility by the way; we're generally ok with breaking compatibility for the purposes of making the library better. |
|
Done |
| public final String filename; | ||
| public final String description; | ||
| public final byte[] data; | ||
| private final String mimeType; |
There was a problem hiding this comment.
Why have you made all of these private? We have a preference for making fields public if they're final, rather than having getter methods. Ditto in other files (including the new BinaryFrame file).
|
Changes look good; thanks. Please can we go back to having public final fields though? If you could revert the indentation changes too, and make the indentation consistent with the rest of the library, that would be great. Not to worry if you can't be bothered though; we can do it after it's merged if needed. NB - Regular indent is 2 spaces, or 4 spaces when breaking a line. |
|
Sure, I will correct that on Monday |
|
Public fields are back |
|
Thanks! |
------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=117224172
This pull request change representation of ID3 data from Map<String,Object> to Id3Tag class. Id3Tag implements Map<String,Object> for backward compability.
However for TXXX frames Id3Tag class have separate Set collection, and because of that Id3Parser was also changed to put all TXXX inside Set and not inside Map itself.
According to ID3v2.3.0 documentation "There may be more than one "TXXX" frame in each tag", previous implementation was overriding TXXX key value and in the end only last TXXX was included in Map object.