Zeugma440 / atldotnet

Fully managed, portable and easy-to-use C# library to read and edit audio data and metadata (tags) from various audio formats, playlists and CUE sheets
MIT License
466 stars 63 forks source link

Not reflecting chapter title changes made with MP3Tag anymore #239

Closed Shrimperator closed 9 months ago

Shrimperator commented 11 months ago

The problem

I've been using an older version of ATL for a while with AudiobookSuite and recently updated. I've since confirmed user reports, that chapter title changes with MP3Tag aren't reflected in my application anymore.

Specifically, ATL's Track.Chapters "Title" property still contains the old data, before the changes in MP3Tag were made.

I'm not sure how this works, or whether this is an issue with ATL or MP3Tag. Since this did work correctly on older versions of ATL, I'm starting here though :)

Environment

I went through the different versions of ATL and it seems like the change was made between 4.9.0 and 4.10.0. Everything prior to 4.10.0 seems to return the correct title strings.

Details

To reproduce:

Code To Reproduce Issue

Track atlTrack = new Track(audioFilePath);

for (int i = 0; i < atlTrack.Chapters.Count; i++)
{
    var chapter = atlTrack.Chapters[i];
    Console.WriteLine(chapter.Title) // <-- different output depending on ATL version and MP3Tag changes
}

Cheers, and thanks for the library. It's incredibly useful!

Zeugma440 commented 11 months ago

Hi and thanks for your feedback 😄 I'm excited to hear that yet another project uses ATL under the hood !

For starters, the culprit is here : https://github.com/Zeugma440/atldotnet/commit/c57c9c7c586e4b987ee92992ed427852954ab579

The issue in details

The MP4/M4A format has two different formats for storing chapters, as described in the dedicated wiki page.

However, all softwares do not support both :

Hence, on an MP4/M4A file tagged with QT chapters, the following happens :

The solution

Up until v4.9.0, ATL blindly replaced QT chapter values with Nero chapter values, which was plain dumb and caused issues.

Since v4.10.0, ATL only replaces QT with Nero chapter values when the latter features more chapters, which is better but has its flaws, as we can see with your report.

The main underlying problem is that we have no way of detecting which chapter format has been added / modified last, as they do not feature any "modification date" field.

We could fix your issues by adding a global Setting to tell ATL if it should look into QT or Nero first :

What do you think about that?

Shrimperator commented 11 months ago

Thank you so much for the detailed explanation! I had no idea there were two different formats in place for chapter data. Though now it makes sense why the old data would even still be part of the files after running them through MP3Tag.

It's an interesting conundrum. I think a way to choose would be great to allow for a bit more compatibility.

The global setting sounds like a good solution to me. Not sure if MP3Tag has ways of removing chapters that might invalidate the output again (because if there's fewer Nero chapters, ATL would then override those with QT again, right?), but even then: that's a very specific edge case and probably irrelevant.

Maybe make the setting an enum and allow to choose between something like: prefer_nero, prefer_quicktime, nero, quicktime (the latter two would forgo overriding entirely.) Though I might be overthinking things here.

Out of curiosity: what kinds of issues were caused by overriding QT with Nero previously? If most programs use QT chapters anyways (and potentially write BS in the Nero chapters), then I definitely wouldn't want to switch back to prefering Nero, right? The only thing I can think of in that case, would really be to give my app's power-users an option inside my app to switch between the two different formats and let them make the decision themselves depending on their library.

I think the settings solution is the best approach in any case.

Zeugma440 commented 11 months ago

Thank you so much for the detailed explanation!

You're very welcome 😉 I'll get to it as soon as I can. The end of year being quite frantic, you can expect a fix in the very beginning of january.

Not sure if MP3Tag has ways of removing chapters that might invalidate the output again (because if there's fewer Nero chapters, ATL would then override those with QT again, right?), but even then: that's a very specific edge case and probably irrelevant.

Best thing you could do would be to open a request on their users forum : https://community.mp3tag.de/

Out of curiosity: what kinds of issues were caused by overriding QT with Nero previously? If most programs use QT chapters anyways (and potentially write BS in the Nero chapters), then I definitely wouldn't want to switch back to prefering Nero, right?

The initial problem that was fixed in v4.10.0 came from a user who reported the opposite issue as yourself, i.e. a file with both formats (Nero and QT) who used an app that only supports QT chapters, leaving obsolete, unedited data in the Nero part. I should've had the idea to create the switch we're talking about back then.

The only thing I can think of in that case, would really be to give my app's power-users an option inside my app to switch between the two different formats and let them make the decision themselves depending on their library.

That's a good approach 👍

Zeugma440 commented 11 months ago

Settings.MP4_readChaptersExclusive has been renamed to Settings.MP4_readChaptersFormat

New values are :

See https://github.com/Zeugma440/atldotnet/commit/acf5d73bec0d44a2b7d76bae62bd0c0c6a3a8d51#diff-6c07ed75e792316a5492c03d9a6dcd442640ff80086627984a22df056b897237

The fix will be available in next release, hopefully very soon.

Zeugma440 commented 11 months ago

Fix is available in today's v5.13. Please close the issue if it works for you~

Zeugma440 commented 9 months ago

Delivered one month ago without feedback => Closed