Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Proposal] Add missing tags from audible aax files #121

Closed
JKamsker opened this issue Jan 5, 2022 · 18 comments
Closed

[Proposal] Add missing tags from audible aax files #121

JKamsker opened this issue Jan 5, 2022 · 18 comments
Assignees

Comments

@JKamsker
Copy link

JKamsker commented Jan 5, 2022

The problem

When opening a .aax file, many fields wont be filled out. Instead, track.AdditionalFields has alot of unmapped properties.
Lurking throught the decompiled sources of Audible/AAXSDKRT and some guesswork, i found out what most of the tags mean.

The resulting model of the sample aax file:

{
    "LONG_DESCRIPTION": "It\u0027s the perfect listen for your morning commute! In the time it takes you to get to work, you\u0027ll hear a digest of the day\u0027s top stories, prepared by the editorial staff of The New York Times. Each edition includes articles from the front page, as well as the paper\u0027s international, national, business, sports, and editorial sections.",
    "SHORT_TITLE": "NYT 07-10-2015 Audio Digest",
    "ASIN": "B00VM3HQAQ",
    "TYPE": "ADBL",
    "VERSION": "158766",
    "GUID": "A2U1HTLPI1WVTV",
    "AACR": "CR!CD9G72PRJD0454R03MXPF6R38HCC",
    "ProductId": "NB_NYTS_150710",
    "Publisher": "The New York Times",
    "Narrator": "The New York Times",
    "ReleaseDate": "10-JUL-2015",
    "PARENT_PRODUCT_ID": "NB_NYTS_000001",
    "PARENT_TITLE": "The New York Times Audio Digest",
    "Parent_ASIN": "B002V9ZD4G"

    "AUTHOR": "",
    "CATEGORY": "",
    "COPYRIGHT": "",
    "COVER_ART": "",
    "PRODUCT_ID": "",
    "PROVIDER": "",
    "PUBLISH_DATE": "",
    "SHORT_DESCRIPTION": "",
    "SUBCATEGORY": "",
    "TITLE": "",
    "USER_ALIAS": "",
    "PARENT_SHORT_TITLE": "",
    "TRACK_NUMBER": "",
    "SynchronizedImage": "",
    "Png": "",
}

Environment

  • ATL version (or git revision) that exhibits the issue: Main as of today

Code To Reproduce Issue

Sample file: https://github.com/inAudible-NG/audible-samples/blob/master/Audible_AAX_sample_62689101.aax
My (very raw) approach: https://gist.github.com/JKamsker/d3333c78789b0e878ba4bea9e3d457ea
This is only meant as inspiration for you to implement in this project. I'm not doing this myself, because i dont know your code style enought.

@Zeugma440 Zeugma440 self-assigned this Jan 5, 2022
@Zeugma440
Copy link
Owner

I'd like to suggest a 3-level fix :

1/ I can certainly add make things easier by mapping some of these values to the fields present in IMetaDataIO (e.g. publisher, release date / publishing date, copyright).

2/ I can also add some generic fields that can be used in other file formats to IMetaDataIO (e.g. narrator, product ID)

3/ However, I do not wish to add AAX-specific fields (e.g. user alias, ASIN, subcategory) to IMetaDataIO. They will have to stay in track.AdditionalFields.

Would that be fine to you ?

@JKamsker
Copy link
Author

I agree.

But i guess Asin could be useful for amazon music downloads ...
I'd rank that as equally important as MusicBrainz

@Zeugma440
Copy link
Owner

Already supported by ATL

  • TITLE
  • AUTHOR (= artist)
  • COPYRIGHT
  • COVER_ART
  • CATEGORY (= genre)
  • TRACK_NUMBER

Missing mappings; added for next release

  • LONG_DESCRIPTION
  • Publisher
  • ReleaseDate (= Publishing date)

New fields available through IMetaDataIO (for next release)

  • ProductId

No support (left in track.AdditionalFields as MP4/AAX is the only standard where that field actually exists)

  • Narrator
  • SHORT_TITLE
  • ASIN
  • TYPE
  • VERSION
  • GUID
  • AACR
  • PARENT_PRODUCT_ID
  • PARENT_TITLE
  • Parent_ASIN
  • PROVIDER
  • SHORT_DESCRIPTION
  • SUBCATEGORY
  • USER_ALIAS
  • PARENT_SHORT_TITLE
  • SynchronizedImage
  • Png

@sandreas
Copy link
Contributor

sandreas commented Mar 28, 2022

There are some other tags that would be nice (especially some for audio books, which I'm highly interested in). Here is a huge list of the MP3Tag's mapping tables, which in most cases makes sense to me. The specific tags that I would love to see (I extracted a listing for you, maybe it helps):

// REF_HEADLINE (SuggestedPropertyName): ["ID3v2.3","ID3v2.4","MP4","Matroska","ASF/Windows Media","RIFF INFO"]

  • ALBUMSORT (SortAlbum): ["TSOA","TSOA","soal","T=50 SORT_WITH","WM/AlbumSortOrder",""]
  • ALBUMARTISTSORT (SortAlbumArtist): ["TSO2","TSO2","soaa","T=30","",""]
  • ARTISTSORT (SortArtist): ["TSOP","TSOP","soar","T=30","WM/ArtistSortOrder",""]
  • CONTENTGROUP (Group): ["TIT1","TIT1","©grp","T=30","WM/ContentGroupDescription",""]
  • ENCODEDBY (EncodedBy): ["TENC","TENC","","T=30 ENCODED_BY","WM/EncodedBy",""]
  • ENCODERSETTINGS (EncoderSettings): ["TSSE","TSSE","©enc","T=30","WM/EncodingSettings",""]
  • MOVEMENTNAME (SeriesTitle): ["MVNM","MVNM","©mvn","T=20 TITLE","",""]
  • MOVEMENT (SeriesPart): ["MVIN","MVIN","©mvi","T=20 PART_NUMBER","",""]
  • MOVEMENTTOTAL (SeriesPartsTotal): ["MVIN","MVIN","©mvc","T=30","",""]
  • PODCASTDESC (LongDescription): ["TDES","TDES","ldes","T=30","",""]
  • SUBTITLE (Subtitle): ["TIT3","TIT3","----:com.apple.iTunes:SUBTITLE","T=30","WM/SubTitle",""]
  • TITLESORT (SortTitle): ["TSOT","TSOT","sonm","T=30 SORT_WITH","WM/TitleSortOrder",""]

Here I created a fully unmodified short summary of all property mappings (maybe helpful to check existing mappings)

Notes:

  • SUBTITLE contains an iTunes specific for mp4, but since there is no other one, I think it is ok to use the mapping, although MP3TAG does not contain this - maybe ©st3 would also be sufficient
  • ENCODERSETTINGS contains ©enc , I think it is ok to use the mapping, although MP3TAG does not contain this. Maybe TENC would be a better match, there is also ©sne (Sound engineer)
  • PODCASTDESC is the LongDescription, you mentioned above

Mapping lists and references, I found pretty useful:

@Zeugma440
Copy link
Owner

Thanks for your feedback @sandreas

First things first, don't forget these fields can already be written by the library, using Track.AdditionalFields. If your app is aimed at handling multiple tagging formats (e.g. ID3v2, native M4A tags, Vorbis tags...), adding these fields generically to IMetaDataIO will simplify your work app-side, allowing you to write less code to handle these formats properly.

Secondly, I'm sensitive to your needs as an editor of audiobook software. Related fields that have equivalents in multiple tagging systems will be implemented generically.

However, don't ask me to implement every known metadata, using these huge mapping lists as specification. I really prefer to invest my time in something users actually need.

Last but not least, I did see your hint at Matroska 😁 Let's say I haven't abandoned the idea of making ATL compatible with that spec. I just need to gather enough time to wrap my brain around it and design an efficient approach. MKV is not your average metadata spec, it's clockwork.

@sandreas
Copy link
Contributor

First things first, don't forget these fields can already be written by the library, using Track.AdditionalFields

I know, ATL is very flexible. I'm promoting your library everywhere I can (e.g. Hackernews), because I think it is an awesome piece of software.

If your app is aimed at handling multiple tagging formats (e.g. ID3v2, native M4A tags, Vorbis tags...), adding these fields generically to IMetaDataIO will simplify your work app-side, allowing you to write less code to handle these formats properly.

I already extended Track class with properties to read and write my suggestions directly mapping them to AdditionalFields. Although there is a huge list of improvements in my mind, I did not understand your code well enough to really improve ATL with PRs - that might come later ;)

Secondly, I'm sensitive to your needs as an editor of audiobook software. Related fields that have equivalents in multiple tagging systems will be implemented generically. However, don't ask me to implement every known metadata, using these huge mapping lists as specification. I really prefer to invest my time in something users actually need.

Since I am the author of m4b-tool, I absolutely understand and support this approach. Currently I'm trying to port m4b-tool to a C# app called tone heavily utilizing atldotnet. So tried to collect the 12 properties I really would need, that in my opinion could be added in a generic way in the list above.

However, the big mapping lists are not really helpful, because if a field is not 100% mappable, the mapping approaches differ depending on which library or software is used (Mp3tag, kid3, ffmpeg, VLC, ExifTool, MP4Box, mp4v2, etc.). I would love to have an RFC here, but although there are many best practises, there is no specification.

I thought over creating a JSON file with a searchable HTML-Document to look up the POSSIBLE mappings with notes about how specific implementations look like - so it would be possible to auto generate mapping code (no reason to write code that can be generated), or make it even configurable to be more flexible with an overridable method for MapAdditionalField:

public class MyTrack : Track
{
    // this is new
    public override string? MapAdditionalField(Format format, string key) => format.ID switch
    {
        // Format is not really appropriate, because it should be the TaggingFormat (id3v2, etc.), not the audio file format
        AudioDataIOFactory.CID_MP3 => MapMp3(key),
        AudioDataIOFactory.CID_MP4 => MapMp4(key),
        _ => null
    };
    
    private static string? MapMp3(string key) => key switch
    {
        nameof(AlbumSort)    => "TSOA",
        nameof(Group)    => "TIT1",
        _ => null,
    };
    
    private static string? MapMp4(string key) => key switch
    {
        nameof(AlbumSort)    => "soal",
        nameof(Group)    => "©grp",
        _ => null,
    };
}

But I tell you something... Instead of hacking my Ideas into comments of issues, I'll think over this carefully and submit PRs after understanding your code better. If something is not to your liking, we discuss it in the PRs. Until then I would really appreciate the following 8 Properties in Track:

  • string SortAlbum
  • string SortAlbumArtist
  • string SortArtist
  • string SortTitle
  • string Group
  • string MovementName (or SeriesTitle)
  • string MovementIndex (or SeriesPart - string because of part 2.1, etc.)
  • string LongDescription

@Zeugma440
Copy link
Owner

PS : Don't hesitate to use GitHub Discussions threads to ask about the code. I'll happily answer!

@sandreas
Copy link
Contributor

sandreas commented Apr 1, 2022

PS : Don't hesitate to use GitHub Discussions threads to ask about the code. I'll happily answer!

Ok, here is a huge Post for discussion ;) #133

@Zeugma440
Copy link
Owner

Zeugma440 commented Oct 15, 2022

@sandreas Almost there, but I have a question for you

string MovementIndex (or SeriesPart - string because of part 2.1, etc.)

Knowing that field is represented by an integer in the MP4 format, how would you handle having "2.1" as the input ? Turning it into 21 ? Truncating to 2 ? Ignoring altogether ?

@sandreas
Copy link
Contributor

Knowing that field is represented by an integer in the MP4 format, how would you handle having "2.1" as the input ? Turning it into 21 ? Truncating to 2 ? Ignoring altogether ?

I found that out later after posting this. So my solution was the following:

In words:

  • I ignore the value if it is not an integer
  • I introduced a custom field (TXXX:PART for id3 and ----:com.pilabor.tone:PART for mp4) to store the real value

This is not the solution I liked most, but it works pretty well.

Zeugma440 added a commit that referenced this issue Oct 15, 2022
@Zeugma440
Copy link
Owner

Zeugma440 commented Oct 15, 2022

Thanks for the quick reply. I chose for the MP4 writer to ignore non-integer values while logging a warning.

As you may expect, I don't intend to create non-standard fallback fields.

Anyway, you'll soon have 8 new standard fields to play with 😄

@sandreas
Copy link
Contributor

I chose for the MP4 writer to ignore non-integer values while logging a warning.

I'm fine with that.

As you may expect, I don't intend to create non-standard fallback fields.

Yeah and this is a good decision :-)

Anyway, you'll soon have 8 new standard fields to play with smile

Cool, thank you very much. I have to check how this works together with my own implementation of 19 new fields by just mapping AdditionalFields in a dirty hack attempt... (see here)

I think there is some serious work to do updating atldotnet to benefit from these changes, but I love the fact that now I have 8 new native fields that are internally mapped and have official support.

Great work, as always, thank you very much.

@Zeugma440
Copy link
Owner

Your work there (and earlier post listing these fields and their mapping) helped me go super fast through today's update, thanks a bunch !

I think there is some serious work to do updating atldotnet to benefit from these changes

Any specific feature / pattern in mind ?

@Zeugma440
Copy link
Owner

Available in today's v4.12

@sandreas
Copy link
Contributor

sandreas commented Oct 16, 2022

Available in today's v4.12

Awesome, thank you.

Any specific feature / pattern in mind ?

No, I just have to remove the properties that are now in the base class from my MetadataTrack, remove the mappings and most important I have to test, if something is different from my mapping approach.

Well, while I like the AdditionalFields approach, it is a bit prone to unexpected behaviour. If I set e.g. AdditionalFields["desc"], it will internally be mapped to Track.Description and when Track.Save is called and reading it from file again, there is no AdditionalFields["desc"] any more.

I don't know exactly what happens if I set AdditionalFields["desc"] = null? Is Track.Description removed then? And this is something I have to figure out.

@Zeugma440
Copy link
Owner

Err... editing both a standard field and its equivalent using AdditionalFields wasn't part of the use cases I imagined 😅

I guess I can manage to ignore what is inside AdditionalFields whenever the standard equivalent has a value. How about that ?

@sandreas
Copy link
Contributor

I guess I can manage to ignore what is inside AdditionalFields whenever the standard equivalent has a value. How about that ?

So maybe I'll give you an example to illustrate the problem:

  • I extended Track with MetadataTrack and implemented an interface IMetadata
  • I used the properties MovementName and Movement - you used SeriesTitle and SeriesPart
  • I could just redirect the properties, but SeriesPart is tricky, because it can only contain integer values
  • Leaving my Movement Implementation with a custom additional field, if it is not an integer is no option, because SeriesPart hides the AdditionalFields[...] and so it needs an extra handling

The atldotnet mapping is also only internal, so it's hard to detect which properties are only available via AdditionalFields and which are mapped to named properties.

Another thing that would be very handy and is missing: The Properties cannot be loaded or saved by a string / enum / Collection:

var title = track.GetProperty(TrackProperty.Title);
track.Set(TrackProperty.Title, "My Title");

foreach(var (propertyName, propertyValue) in track.GetAllProperties()) {
    // ...
}

So one solution might be to have just one Dictionary to store everything: Track.Properties<string, object> and only LINK the real Properties of Track to this dictionary - Track.AdditionalFields would no longer be necessary and still everything would have defined behaviour. Only Problem would be to support multiple Metadata formats (like id3v2 + ape).

So what I would do (in the long term):

  • Establish an Enum TagProperty with internal Names for tags (e.g. Title, Album, Movement, etc.)
  • Create a Dictionary<TagProperty, object?> Properties, where object can be string,DateTime, etc.
  • Handle the Mapping for the different formats within the format (e.g. TagProperty.Movement => ©mvi for mp4, TagProperty.Movement => MVIN for id3, etc.)
  • Link all real properties to this dictionary ( public int? Bpm => {get => GetProperty(TagProperty.Bpm); set => SetProperty(TagProperty.Bpm, value);} )
  • Ignore properties, that are not defined for the specific metadata format

For a list of possible internal names, you could take a look at: https://docs.mp3tag.de/mapping/

But maybe this should be a discussion, because this would be a HUGE change (although the interface would basically stay the same)

@Zeugma440
Copy link
Owner

Continued on #167 - the issue itself has been fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants