Incorrect timestamps on Syncronized lyrics

[Sandakan]

I used these settings to update syncronizedLyrics on a song.

{
                contentType: TagConstants.SynchronisedLyrics.ContentType.LYRICS,
        language: 'ENG',
        timeStampFormat: TagConstants.TimeStampFormat.MILLISECONDS,
        shortText: 'this is short text',
        synchronisedText: [
            { text: 'Lyrics line 1', timeStamp: 12.5 },
            { text: 'Lyrics line 2', timeStamp: 14.5 },
            { text: 'Lyrics line 3', timeStamp: 16.5 },
            { text: 'Lyrics line 4', timeStamp: 18.5 },
            { text: 'Lyrics line 5', timeStamp: 20.5 },
        ],
    }

But when I read the same song, I get a different output with incorrect timestamps and the last lyric line missing.

{
        language: "ENG",
        timeStampFormat: 2,
        contentType: 1,
        shortText: "this is short text",
        synchronisedText: [
          {
            text: "Lyrics line 1",
            timeStamp: 4294855680,
          },
          {
            text: "yrics line 2",
            timeStamp: 4294855680,
          },
          {
            text: "yrics line 3",
            timeStamp: 4294855680,
          },
          {
            text: "yrics line 4",
            timeStamp: 4294855680,
          },
        ],
      },

[pbricout]

The value is not the same because timeStamp must be an integer value. If you replace your values with an integer value, the timeStamp will be correct, this will also fix the issue with the missing line.

[Zazama]

Is 12.5 the second? Because you have to pass the millisecond (e.g. 12500 in this case).

This is also partially a bug: https://github.com/Zazama/node-id3/blob/0.2/src/ID3FrameBuilder.js#L17 The call to Number.isInteger(value) is false but no buffer is appended, which makes the frame be out-of-spec. The Ls in line 2-4 (and line 5 alltogether) are missing because they are consumed by the timeStamp (4294855680 => 0xFFFE4C00 => L).

[pbricout]

I created #147 to improve the documentation and prevent silent failure in appendNumber.

[pbricout]

Is 12.5 the second? Because you have to pass the millisecond (e.g. 12500 in this case).

This is also partially a bug: https://github.com/Zazama/node-id3/blob/0.2/src/ID3FrameBuilder.js#L17 The call to Number.isInteger(value) is false but no buffer is appended, which makes the frame be out-of-spec. The Ls in line 2-4 (and line 5 alltogether) are missing because they are consumed by the timeStamp (4294855680 => 0xFFFE4C00 => L).

Yes, this is exactly what I found out.

[Sandakan]

The value is not the same because timeStamp must be an integer value. If you replace your values with an integer value, the timeStamp will be correct, this will also fix the issue with the missing line.

It worked this time. 😁😁

[Sandakan]

There's another problem. Whenever I update the lyrics, it will add a new lyrics object to syncronizedLyrics array instead of updating the previous one. Is there a way to clear the array before writing it again with the new lyrics or should I use this approach and just read the last item in the array?

[pbricout]

Is 12.5 the second? Because you have to pass the millisecond (e.g. 12500 in this case).

Thanks @Zazama for pointing this out, @Sandakan did you see this point?

[pbricout]

There's another problem. Whenever I update the lyrics, it will add a new lyrics object to syncronizedLyrics array instead of updating the previous one. Is there a way to clear the array before writing it again with the new lyrics or should I use this approach and just read the last item in the array?

This is working as designed.

You should notice in the README documentation that the synchronisedLyrics takes an array of definition. From the id3 specs, there can be more than one. Internally, this frame is defined as multiple here.

When doing an update, the API should not modify the other frames. Therefore adds a new one. We understand that a user may want to completely replace all the lyrics. This is a known issue with the current design of the API and is the same for all the frames which can accepts multiple values.

The workaround for now is to not use the update function and instead read all the tags, replace the synchronisedLyrics one and rewrite them. Notice that this may remove some tags from your file that are not supported by this library. But I think we should support almost all the known official tags.

This will be improved in future versions.

[Sandakan]

What if I keep adding new lyrics objects to the array whenever I update the song and read only the last element of the array? This should work, right? I hope that there are no limitations to the number of lyrics objects that can be put into this array.

[pbricout]

As I said I recommended not to use the update function when manipulating lyrics, otherwise you will keep adding lyrics frames to the file, this is most likely not what you want to do. Do a read, update the result and write everything back.

Just to be clear there are two arrays and there is no limitation in neither of them:

{
    synchronisedLyrics: [{ // The top level one, for example one for each language
        language: "ENG",
        timeStampFormat: 2,
        contentType: 1,
        shortText: "this is short text",
        synchronisedText: [ // The synchronised text one
          {
            text: "Lyrics line 1",
            timeStamp: 0,
          },
        ],
    }
}

[Sandakan]

I tested it with some other lyrics.

{
        contentType: TagConstants.SynchronisedLyrics.ContentType.LYRICS,
        language: 'ENG',
        timeStampFormat: TagConstants.TimeStampFormat.MILLISECONDS,
        shortText: 'this is short text',
        synchronisedText: [
            { text: 'Never thought I would love again', timeStamp: 3610 },
            { text: 'Here I am, lost in Love Me Land', timeStamp: 12380 },
            { text: '♪', timeStamp: 20340 },
            { text: 'How dare you have lips like that?', timeStamp: 29400 },
            { text: "How dare you gon' kiss like that?", timeStamp: 34150 },
        ],
    };

And this is the output I got.

{
        language: 'ENG',
        timeStampFormat: 2,
        contentType: 1,
        shortText: 'this is short text',
        synchronisedText: [
            { text: 'Never thought I would love again', timeStamp: 3610 },
            { text: 'Here I am, lost in Love Me Land', timeStamp: 12380 },
            { text: '♪', timeStamp: 5207295 },
            { text: '﹈ow dare you have lips like that?', timeStamp: 29400 },
            { text: "How dare you gon' kiss like that?", timeStamp: 34150 },
        ],
    },

In the third sychronisedText line, the timestamp is incorrect and the line after that has an unknown letter instead of the letter "H".

[Zazama]

https://github.com/Zazama/node-id3/pull/148 @pbricout I tried to fix it, any thoughts before merging?

When merged I can backport to branch 0.2 to release a fix

[pbricout]

@Zazama having a look at #148 .

[Zazama]

@Sandakan You can update to version 0.2.6 and your problem should be fixed

https://www.npmjs.com/package/node-id3/v/0.2.6

[Sandakan]

@Sandakan You can update to version 0.2.6 and your problem should be fixed

https://www.npmjs.com/package/node-id3/v/0.2.6

@Zazama Thanks for the update and the quick fix. 😁😁🎉🎉

[pbricout]

@Sandakan thanks for reporting the bug! Thanks @Zazama for the quick fix!