Add support for TagLib 2.0

[Holzhaus]

This fixes a build error with TagLib 2.0 while still being backwards compatible with legacy TagLib 1.x versions. The binaryData() method exists since TagLib 1.8 which was released almost 12 years ago (September 6, 2012).

Also see https://taglib.org/#taglib-20-release---jan-24-2024:

New major version, binary incompatible, but source-compatible with the latest 1.x release if no deprecated features are used.

This seems to be the only deprecated API we use, at least it builds fine on Arch.

Obsoletes #12709 and #12760.

[toszlanyi]

Compiles successfully on my EndeavourOS (Arch) box - only taglib2 installed, no taglib1.
I changed and exported metadata of one file in a crate successfully as well.
Anything else to test specifically?

[Holzhaus]

The only change is the method used for reading cover art from APE tags, so this makes sense to test.

[daschuer]

Thank you for taking care. It is a good idea to remove deprecated calls from our code base.

However this is not true IMHO:

Obsoletes #12709 and #12760.

Because Taglib 2.0 also changes the way tags are handled. There is at least a new separator " / " for multi value tags and probably other braking changes. We have a good experience with wired issue around taglib including crashes.

#12760 helps to have a free choice between V1 and V2 which is also a good idea in any case.

At least this is not a no-brainer and I would aeaidvthe risk a few days before our 2.4.0 release.

Can you please revert removing the guard against 2.0?

[daschuer]

Please note that Taglib has also change the advice to

New major version, binary incompatible, but mostly source-compatible with the latest 1.x release if no deprecated features are used. Simple applications should build without changes, more complex applications (e.g. extending classes of TagLib) will have to be adapted.

This is a good indicator that it is more safe to let package maintainers stick 1.13 for 2.4.0 until we have a done a good amount of testing and maybe until 2.1 is released.

[daschuer]

Did you consider what needs to be done for the new separator?

[Holzhaus]

Because Taglib 2.0 also changes the way tags are handled. There is at least a new separator " / " for multi value tags and probably other braking changes. We have a good experience with wired issue around taglib including crashes.

I had a look and grepped the code a bit, but I doesn't look like we handle multi-valued fields anyway (at least from my reading of the mixxx code).

#12760 helps to have a free choice between V1 and V2 which is also a good idea in any case.

[...]

Can you please revert removing the guard against 2.0?

I don't understand. The PR you mention adds some workarounds for supporting the self-built taglib1 on Arch Linux. It does not remove the guard against using TagLib 2.0. The entire purpose of the guard is to make it impossible to build Mixxx with TagLib 2.0, and you want to keep it that way. So how does that PR "help to have a free choice"?

If I understand correctly, #12760 is only needed for Arch Linux. We should send that patch to the Arch maintainer and may apply it to the AUR packages but it doesn't really belong into the Mixxx codebase. We shouldn't try to add workarounds for specific distros, otherwise I worry that it will degrade the code quality in the long term.

Regarding the removal of the guard, there are two major reasons for it:

1. I agree that we should recommend using TagLib 1.x for now due to the lack of testing. We should mention that in the release announcement and it might even warrant a warning printed at build time. But the fatal error without no way to override it when trying to build Mixxx with TagLib 2.0 is way too intrusive IMO. It made sense when it was added because the build would fail anyway and trying to build would just be a waste of time. But now the build works. If the user wants to use Mixxx with TagLib 2.0 despite being warned, it should be possible.
2. The guard does not work. I am able to build Mixxx with TagLib 2.0 even without reverting the addition of the guard. If the guard worked I would have just downgraded it to a warning instead of removing it, but since the code is not working anyway, it does not make sense to keep it.

At least this is not a no-brainer and I would aeaidvthe risk a few days before our 2.4.0 release.

This does not influence our pre-built binaries. The only risk I see is with distro maintainers. However, we could alleviate that with a remark in the release notes to use TagLib 1.x and maybe an additional warning when building Mixxx.

[daschuer]

Can you please revert removing the guard against 2.0?

I don't understand. ...

My idea was to keep the guard for the 2.4.0 release to not put user data on a risk of an untested library with Mixxx. We should remove it with 2.5, than we have the 2.5-beta phase to become confident that the library is good. We may also consider to remove the guard in 2.4.1

If I understand correctly, #12760 is only needed for Arch Linux.

It is needed for all distros that integrate Taglib 2.0 without an extra namespace. The path of taglib1 might be ARCH specific, but it can be adjusted via PKG_CONFIG_PATH. We don't know which decision other bistros make, but it looks like at least freeBSD will follow.

But now the build works. If the user wants to use Mixxx with TagLib 2.0 despite being warned, it should be possible.

Flowing the 2.5 idea, the user can just remove the FATAL_ERROR line or use the 2.5 branch. We may add an extra config value, but I don't think it is worth the effort to spend time time for making things convenient that we do not want.

The guard does not work.

Than let's repair it.

The only risk I see is with distro maintainers.

We have the experience that a warning is not strict enough,

[daschuer]

My original Idea was also to just focus on the release and not spend time for the taglib1/2 issue we do now. So let's take here a safe shortcut and continue this discussion in a week or such after 2.4.0 is released.

[daschuer]

The guard does not work.

Than let's repair it.

The 2.4 branch will use the 2.0 header even if it will link against taglib 1.0 because of the taglib prefixed includes.
This is also fixed in #12760 Is that the issue you are referring to?

Or is it a CMakeCache issue?

[Holzhaus]

I don't have taglib1 installed. Mixxx builds without issues. I didn't investigate it.

[daschuer]

Interesting, on which OS are you? Does cake display a version? I can imagine that the version lookup fallback is broken when ther is no PC file found.

[mxmilkiib]

FWIW, I just did a full official and AUR update, build of main is still failing;

[ 75%] Building CXX object CMakeFiles/mixxx-lib.dir/src/waveform/renderers/glvsynctestrenderer.cpp.o
/home/milk/pkgs/mixxx-git-debug/src/mixxx/src/track/taglib/trackmetadata_ape.cpp: In function ‘bool mixxx::taglib::ape::importCoverImageFromTag(QImage*, const TagLib::APE::Tag&)’:
/home/milk/pkgs/mixxx-git-debug/src/mixxx/src/track/taglib/trackmetadata_ape.cpp:62:56: error: ‘const class TagLib::APE::Item’ has no member named ‘value’; did you mean ‘values’?
   62 |                 tag.itemListMap()["COVER ART (FRONT)"].value();
      |                                                        ^~~~~
      |                                                        values
make[3]: *** [CMakeFiles/mixxx-lib.dir/build.make:5718: CMakeFiles/mixxx-lib.dir/src/track/taglib/trackmetadata_ape.cpp.o] Error 1
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [CMakeFiles/Makefile2:243: CMakeFiles/mixxx-lib.dir/all] Error 2
make[1]: *** [CMakeFiles/Makefile2:302: CMakeFiles/mixxx.dir/rule] Error 2
make: *** [Makefile:212: mixxx] Error 2
==> ERROR: A failure occurred in build().

[toszlanyi]

The only change is the method used for reading cover art from APE tags, so this makes sense to test.

giving up on that - couldn't find nor convert a track to APE file format. Sorry.

[daschuer]

@mxmilkiib to test this, you need to checkout the branch of this PR:

git checkout 2.4
git checkout -b Holzhaus-taglib-2.0
git pull https://github.com/Holzhaus/mixxx.git taglib-2.0

[daschuer]

Removing the guard will introduce this issue:
#12790

[mxmilkiib]

@mxmilkiib to test this, you need to checkout the branch of this PR:

git checkout 2.4
git checkout -b Holzhaus-taglib-2.0
git pull https://github.com/Holzhaus/mixxx.git taglib-2.0

Sure, excuse me for not testing more in specific or in general yet, but I haven't worked out a non frustrating workflow for building (whether it should involve a PKGBUILD at all, which neatly contains all the build steps and parameters, and, a more recent concern, if I can easily integrate GitButler with it's oh-so-pain-point-reducing virtual branch feature), though hopefully I can sort that out today.

[daschuer]

I use git worktree for that purpose as far as I understand you. That works out of the box and is fast and convenient. The only issue is that it is not allowed to checkout the same branch twice.

[Holzhaus]

@mxmilkiib to test this, you need to checkout the branch of this PR:

git checkout 2.4
git checkout -b Holzhaus-taglib-2.0
git pull https://github.com/Holzhaus/mixxx.git taglib-2.0

Sure, excuse me for not testing more in specific or in general yet, but I haven't worked out a non frustrating workflow for building

If you're using a PKGBUILD, just replace the Mixxx repository URL in sources with git+https://github.com/Holzhaus/mixxx.git#branch=taglib-2.0

See https://man.archlinux.org/man/PKGBUILD.5#USING_VCS_SOURCES for details.

[Holzhaus]

I removed the revert to unblock this fix, and will create a separate PR.

[daschuer]

Thank you.

[mxmilkiib]

The git worktree feature is well cool n nice.

If you're using a PKGBUILD, just replace the Mixxx repository URL in sources with git+https://github.com/Holzhaus/mixxx.git#branch=taglib-2.0

I do that, but it complains about the local repo not being the same as the source repo, and I have to delete the bare mixxx/ repo and the extracted src/ so makepkg can pull as fresh copy, which is the frustrating part.

I don't know how to reconcile things, so I guess I'll just forsake the "building a package and installing a proper pacman tracked instance" part of the process that makepkg provides, and use the manual untracked install command method that I'll take from the PKGBUILD install().

[daschuer]

The good thing with Mixxx is that it does not require to be installed. It runs from the build directory.
The only issue is that it uses the same user setting and databaseif the --settingsPath command line argument is not used.

So in your case, just clone mixxx repository to your home drive and use cmake as described in our wiki.