Avoid reading the whole file into memory when updating ID3 tag of a file

[fabiankr]

Currently, when updating the ID3 tag of an existing file the whole file is read into memory. This PR aims to avoid that in order to keep memory consumption low.

Description

Instead of reading the whole MP3 file as Data into memory use file handles to read only the current ID3 tag from the existing file (implemented in #99), then write the new tag and read the rest of the existing file (the audio data) in chunks of 64 KB.

Motivation and Context

I'm using this library to update the ID3 tag of audio files when importing them into my app. While working on a share extension I noticed that the extension was killed by the system for using too much memory when saving the tag of a large MP3 file (greater than 60 MB). With this change that no longer happens.

Apps may use more memory than extensions, but even for apps this change is good because it will lower the memory pressure of the devices.

How Has This Been Tested?

Tested in my app with hundreds of MP3 files, from small (10 MB) to large (300 MB).

Types of changes

• Bug fix 🐛 (non-breaking change which fixes an issue)
• New feature ✨ (non-breaking change which adds functionality)
• Breaking change 💥 (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project 🍻.
• My change requires a change to the documentation 💡 and I have updated the documentation accordingly.
• I have read the CONTRIBUTING document 👥.
• I have added tests to cover my changes 🎉.
• All new and existing tests passed ✅.

[fabiankr]

Currently, a lot of existing tests fail. However, this seems to be unrelated to the change. Somehow the tag size of the example-to-be-modified.mp3 file can't be read correctly and therefore let currentId3Tag = try read(from: path) fails.

I have added a failing test that shows the root of the problem:

@Test func parseTagSizeToBeModified() {
    let mp3 = NSData(contentsOfFile: PathLoader().pathFor(name: "example-to-be-modified", fileType: "mp3"))!
    #expect(ID3TagSizeParser().parse(data: mp3) < mp3.count)
}

Maybe you have an idea why this file can't be read correctly.

[chicio]

Hi @fabiankr and thanks for the contribution 🙏

I will try to have a look at your PR in the next days and let you know which is the problem. Let's see if we can merge this in the next weeks.

[chicio]

It seems that the problem rely in this change. If you rollback to

        let mp3 = try mp3FileReader.readFileFrom(path: path)
        let currentId3Tag = try self.id3TagParser.parse(mp3: mp3)

Everything should be fine. I will investigate deeply after the merge if there are problems with the examples (because generally speaking other library are not so strict in the check as this one I did, and some of the mp3 where generated using them. So probably, when I created this example, they came as output from another library).

Can you verify that by reverting this part your implementation is still fine? This will be a BIG improvements for the library (thanks again for the contribution 🙏 )

[fabiankr]

readFileFrom(path:) reads the whole file into memory, so I can't just revert that line. However, I think the problem is that read(from:) doesn't check for the ID3 tag presence first using isTagPresentIn(mp3:version:). So it tries to parse the tag size even though there's no ID3 tag at all. I'll fix it.

[chicio]

@fabiankr I added a couple of comments explaining the situation. Let me know if it is clear 🚀

[chicio]

Currently, a lot of existing tests fail. However, this seems to be unrelated to the change. Somehow the tag size of the example-to-be-modified.mp3 file can't be read correctly and therefore let currentId3Tag = try read(from: path) fails.

I have added a failing test that shows the root of the problem:

@Test func parseTagSizeToBeModified() {
    let mp3 = NSData(contentsOfFile: PathLoader().pathFor(name: "example-to-be-modified", fileType: "mp3"))!
    #expect(ID3TagSizeParser().parse(data: mp3) < mp3.count)
}

Maybe you have an idea why this file can't be read correctly.

As mentioned in the CR comments, probably that example come from another library (I mean I generated it using another library/tool, like "Mp3Tag" app or a java/c++ library), that are not as strict as ID3TagEditor on the checks to be done. Let's try apply the suggestion I made to see if everything works fine.

[fabiankr]

Currently, a lot of existing tests fail. However, this seems to be unrelated to the change. Somehow the tag size of the example-to-be-modified.mp3 file can't be read correctly and therefore let currentId3Tag = try read(from: path) fails.
I have added a failing test that shows the root of the problem:

@Test func parseTagSizeToBeModified() {
    let mp3 = NSData(contentsOfFile: PathLoader().pathFor(name: "example-to-be-modified", fileType: "mp3"))!
    #expect(ID3TagSizeParser().parse(data: mp3) < mp3.count)
}

Maybe you have an idea why this file can't be read correctly.

As mentioned in the CR comments, probably that example come from another library (I mean I generated it using another library/tool, like "Mp3Tag" app or a java/c++ library), that are not as strict as ID3TagEditor on the checks to be done. Let's try apply the suggestion I made to see if everything works fine.

So turns out the example-to-be-modified.mp3 file has no ID3 tag at all and the function read(from:) doesn't check for the presence of the tag before calculating the tag size from the (apparent) header data.

I fixed the reading function to perform the check and made some slight refactoring. Now all tests are green. 👌
Please double-check and let me know if you want me to change anything.

[chicio]

Currently, a lot of existing tests fail. However, this seems to be unrelated to the change. Somehow the tag size of the example-to-be-modified.mp3 file can't be read correctly and therefore let currentId3Tag = try read(from: path) fails.
I have added a failing test that shows the root of the problem:

@Test func parseTagSizeToBeModified() {
    let mp3 = NSData(contentsOfFile: PathLoader().pathFor(name: "example-to-be-modified", fileType: "mp3"))!
    #expect(ID3TagSizeParser().parse(data: mp3) < mp3.count)
}

Maybe you have an idea why this file can't be read correctly.

As mentioned in the CR comments, probably that example come from another library (I mean I generated it using another library/tool, like "Mp3Tag" app or a java/c++ library), that are not as strict as ID3TagEditor on the checks to be done. Let's try apply the suggestion I made to see if everything works fine.

So turns out the example-to-be-modified.mp3 file has no ID3 tag at all and the function read(from:) doesn't check for the presence of the tag before calculating the tag size from the (apparent) header data.

I fixed the reading function to perform the check and made some slight refactoring. Now all tests are green. 👌 Please double-check and let me know if you want me to change anything.

Ok so I was probably out of my mind when I reviewed the changes 🤣 I reviewed the new changes and they seem fine, but there are still some errors on tvOS and linux (check the pipeline results). I think it is just a matter of conditionally change the code for tvOS (like you did for iOS) + removing the autoreleasepool conditionally when running on linux platforms (https://forums.swift.org/t/autoreleasepool-for-ubuntu/4419/17).
After all the pipelines are green, we can merge.

PS if you need to run ID3TagEditor on linux, there is a script in the scripts folder to run a docker instance of the swift image with the folder of the code mounted as volume (check also additional infos here https://www.fabrizioduroni.it/2021/05/31/swift-linux-test-local-ci-docker-container/)

[fabiankr]

Thanks for the links! Yeah I didn’t check the correct OS versions for the deprecations yet. I’ll fix it and also look at the Linux build. 👍

[fabiankr]

@chicio All tests are now green. 👍

[chicio]

@chicio All tests are now green. 👍

Very good 🚀 I will merge and release it this evening or tomorrow evening. Thanks again for the contribution, I really appreciate it 🙏

[chicio]

@fabiankr version 5.2.0 published with your contribution. The GitHub actions pipelines added also you to the contributors 🚀 🚀 🚀 🚀 🚀