Review Request 112266: Fix reading Album Artist / Compilation / Disc Number in APE tags

[Bruno Léon]

> On Aug. 25, 2013, 12:06 p.m., Matěj Laitl wrote:
> > shared/tag_helpers/APETagHelper.cpp, lines 33-37
> > <http://git.reviewboard.kde.org/r/112266/diff/1/?file=184486#file184486line33>
> >
> >     Hmm, is there a specification somewhere that says what the proper identifiers are? I fear of backwards compatibility, perhaps there are files out there that use the title-cased identifiers?
> 
> Bruno Léon wrote:
>     I did not find specifications for this.
>     Actually when using "Album Artist" instead of "ALBUM ARTIST", the tag is not read at all.
>     Taglib output it as "ALBUM ARTIST" which made me make this correction (and checked it actually works).
>     
>     Same this for compilation. This makes Amarok compliant with Taglib output format.
> 
> Matěj Laitl wrote:
>     I don't think that TagLib itself is origin of the string, I think it just reads the identifier that is in the actual tag (which can vary file by file). Or perhaps you can prove me wrong by pointing out relevant part of TagLib source code?
> 
> Mark Kretschmann wrote:
>     Here's the spec: http://wiki.hydrogenaudio.org/index.php?title=APE_Tag_Item
>     
>     And here is the list of valid keys: http://wiki.hydrogenaudio.org/index.php?title=APE_key
>     
>     Keys are case sensitive, first character uppercase, rest lowercase (e.g. "Artist"). So it looks like the all-uppercase stuff must come from TagLib, and that our original code used the keys from the actual spec (minus "Album Artist" and "BPM", which don't exist in the spec at all).
> 
> Mark Kretschmann wrote:
>     Ergo, probably Matej is right, TagLib just reads the identifiers from the file, and the files you have are actually incompatible with the spec. We could simply toLower() the strings before comparing.

Taglib doc (http://taglib.github.io/api/classTagLib_1_1APE_1_1Tag.html) states that key are case-insensitive.
I'm not sure that Hydrogen Audio page is really "the spec", these are just commnly used keys.

I attached a sample output from Taglib.
As Matej says it probably just reads the identifier that is in the tag, but it outputs in UPPER CASE.

What I can assure is that if I tag my file either with MusicBrainz or directly in python using mutagen, without my proposed modifications discussed tags are not seen in Amarok.


- Bruno


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112266/#review38527
-----------------------------------------------------------


On Aug. 25, 2013, 11:17 a.m., Bruno Léon wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112266/
> -----------------------------------------------------------
> 
> (Updated Aug. 25, 2013, 11:17 a.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> Fix reading of Album Artist and Compilation tag in APE tags.
> Add support for reading Disc Number in APE tags.
> 
> 
> Diffs
> -----
> 
>   shared/tag_helpers/APETagHelper.cpp c628694 
> 
> Diff: http://git.reviewboard.kde.org/r/112266/diff/
> 
> 
> Testing
> -------
> 
> Tested with Musepack files (that do use APE tags)
> 
> 
> Thanks,
> 
> Bruno Léon
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130825/2b93c01f/attachment-0001.html>