Remove MP3File.id3v2Asv24tag

[Dan Gravell]

MP3File maintains a member id3v2Asv24tag which is a representation of the current ID3v2 tag, adapted to be an ID3v24Tag. When setID3v2Tag is called the type of the argument is checked, and if it isn’t an ID3v2.4 tag it creates a representation, copying the data from the original tag.

The trouble with this is that some fields can take up a lot of memory. Large album art for example can be multiple MBs in size. Multiplied by the number of tracks that are read into memory, this can amount to a non-trivial amount.

Added to this, it doesn’t appear id3v2Asv24tag is actually used anywhere, outside of unit tests.

Should id3v2Asv24tag be removed? If not, would it not be better to have something like a flyweight in its place? https://refactoring.guru/design-patterns/flyweight

[IJabz]

Hi, well I do use it in Jaikoz, I use it when I save changes back to the Mp3, perhaps I can change the way I do this (I dont use it in SongKong) but it is used.

But anyway the max space this can take up is double (also note if the Mp3File is already using ID3v24 then it doesnt use any addtional space, only when using ID3v23 or ID3v22), so I am just wondering isnt the issue that are you holding multiple MP3Files in memory, you know if we fixed this issue the maximum amount of memory recovered will only be half the memory used by the MP3File. Having read info from an Mp3File shouldn't you be releasing the Mp3File object ?

[IJabz]

Your proposed solution of using Flyweight pattern looks fine, but this is probably non trivial to achieve.

[Dan Gravell]

Reading from disk is costly. Really costly. Especially across a network. So where possible I cache any file that is read. The more memory a file takes up, the fewer I can fit in the cache, and so the more disk reads I have to do.

you know if we fixed this issue the maximum amount of memory recovered will only be half the memory used by the MP3File

Album art quite commonly comes in at over 1MB these days. If we multiply that by the number of tracks in an album, then consider we might be running on memory constrained devices… that becomes a fairly big deal.

Imagine a 20-odd track album running on a NAS with 128MB heap spare (maybe it’s a 0.5GB device).

How about just generating the ID3v2.4 tag lazily when required, rather than eagerly at construction time?

[Dan Gravell]

My current workaround is:

AudioFile audioFile = ...
MP3File mp3File = (MP3File) audioFile;
Optional<AbstractID3v2Tag> optId3v2 = Optional.ofNullable(mp3File.getID3v2Tag());
optId3v2.ifPresent(id3v2Tag -> {
  mp3File.setID3v2TagOnly(id3v2Tag);
});
return mp3File;

setID3v2TagOnly unsets the ID3v2.4 tag so the memory is saved a bit. Obviously we still incur the up-front cost of allocating time and storage to creating the ID3v2.4 tag.

[Dan Gravell]

I forgot that another issue with this copy object is that it becomes out of date when anything on the “actual” ID3v2 tag is changed.

Smells of duplication…

[IJabz]

I dont really disagree with your approach, but its not a priority for me.

All Im saying is why are you caching the MP3File at all, why dont you just read the data you are interested in into your own structure and release Mp3File, and then you only need to retrieve the MP3File if you actually want to save data to it at a later date.

[Dan Gravell]

Quite often I want to read a file, work out what needs doing to it, then update it and write it back in an automated fashion.

Reading the file twice seems wasteful.