mpatric / mp3agic

A java library for reading mp3 files and reading / manipulating the ID3 tags (ID3v1 and ID3v2.2 through ID3v2.4).
MIT License
1.2k stars 310 forks source link

getYear() returning null #156

Open klausenbusk opened 5 years ago

klausenbusk commented 5 years ago

Hi

I'm using mp3agic as part of my Java music player project, it works well, but getYear always returns null with my test data.

So I tried reading the year with the mp3agic exampe to rule out any programmatic errors from my side. Running the example I get:

Length of this mp3 is: 96 seconds
Bitrate: 182 kbps (VBR)
Sample rate: 44100 Hz
Has ID3v1 tag?: YES
Has ID3v2 tag?: YES
Has custom tag?: NO
Track: 1
Artist: Moped Genius
Title: Fall In Love
Album: Multiverse
Year: 2015
Genre: -1 (Unknown)
Comment: 
Track: 1
Artist: Moped Genius
Title: Fall In Love
Album: Multiverse
Year: null
Genre: -1 (indiepop)
Comment: http://www.jamendo.com cc_standard
Composer: Moped Genius
Publisher: http://www.jamendo.com
Original artist: null
Album artist: null
Copyright: http://creativecommons.org/licenses/by-nc-nd/3.0/
URL: null
Encoder: Jamendo:http://www.jamendo.com| LAME
Mime type: image/jpeg

As you can see, getYear returns null. I also tried reading the id3 tags with another library (easytag which use id3lib), and it works perfectly (I get year 2015).

Test data:

MopedGenius-_Fall_In_Love.zip Downloaded from: https://www.jamendo.com/track/1562762/fall-in-love (License)

- Kristian

hennr commented 5 years ago

Hi @klausenbusk,

I just had a quick debugging session an saw that the file has the record date set but not the year.

Following test code shows what I mean:

        byte[] buffer = TestHelper.loadFile("src/test/resources/Moped_Genius_-_Fall_In_Love.mp3");
        ID3v24Tag id3v24tag = (ID3v24Tag) ID3v2TagFactory.createTag(buffer);
        assertEquals("2015", id3v24tag.getRecordingTime());
klausenbusk commented 5 years ago

I just had a quick debugging session an saw that the file has the record date set but not the year.

Oh. According to id3info TYERis set. Should ID3v2 override the getYear method and call getRecordingTime on ID3v2{2...4}Tag? ID3v2{2..3} does not contain a getRecordingTime method at the moment, but it can be added.

klausenbusk commented 5 years ago

According to id3info TYERis set. Should ID3v2 override the getYear method and call getRecordingTime on ID3v2{2...4}Tag? ID3v2{2..3}' does not contain agetRecordingTime` method at the moment, but it can be added.

So getYear should return recording year.

What do you think? I'm willing to open a PR if it sounds good to you?

hennr commented 5 years ago

I just had a quick debugging session an saw that the file has the record date set but not the year.

Oh. According to id3info TYERis set.

If I remember correctly TYER was probed but it was not set. A look with a hex code editor comes to the same result.

Question is if getYear() should fall back to TDRC if TYE and TYER are not set but I tend to not doing so as it is a different field. This could be handled by the application itself, if desired.

Everyone ok with this? @mpatric @tmzkt

tmzkt commented 5 years ago

I don't like the fact that ID3v24Tag allows users to get/set TYER and TDAT - those are not valid ID3 v2.4 frames. They should be using TDRC. There are two ways I can think to solve this:

  1. remove getYear, setYear, getDate, and setDate from ID3v24Tag and force users to use getRecordingTime and setRecordingTime
  2. override getYear, setYear, getDate, and setDate in ID3v24Tag to manipulate TDRC (and never TYER and TDAT)

It is probably best to go with option 2 so that the interface remains consistent between ID3v22Tag, ID3v23Tag, and ID3v24Tag.