Version 4.1.4/4.1.5 breaks applications written in C (such as cmus)

[X-m7]

The line below is only valid in C++ (introduced in commit 2637194), so if a C application tries to use the library it fails to compile, whereas with version 4.1.3 it works just fine. cmus in particular is affected by this change.

mp4v2/include/mp4v2/file.h

Line 336 in 67ed6f3

ShouldParseAtomCallback cb = nullptr );

[Korynkai]

I can confirm this also breaks Freeswitch mod_mp4v2 build:

making all mod_mp4v2
make[4]: Entering directory '/startdir/src/freeswitch-1.10.5/src/mod/applications/mod_mp4v2'
  CC       mod_mp4v2_la-mod_mp4v2.lo
In file included from /usr/include/mp4v2/mp4v2.h:58,
                 from mod_mp4v2.c:37:
/usr/include/mp4v2/file.h:336:32: error: expected ‘;’, ‘,’ or ‘)’ before ‘=’ token
  336 |     ShouldParseAtomCallback cb = nullptr );
      |                                ^
make[4]: *** [Makefile:714: mod_mp4v2_la-mod_mp4v2.lo] Error 1
make[4]: Leaving directory '/startdir/src/freeswitch-1.10.5/src/mod/applications/mod_mp4v2'

[enzo1982]

Introducing API/ABI breaking changes in a patch release is really bad practice. This should have been done by adding a new function, like MP4Read2, with the additional argument. The way this has been done here, it not only breaks the API for C applications, but also breaks the ABI for every consumer of the MP4Read function, so the MP4v2 library cannot be updated from 4.1.3 to 4.1.4 or later without recompiling each and every dependency.

I understand that this repository contains a libmp4v2 fork for internal use at TechSmith and you're just publishing it here to comply with the license. In that case, you can of course break things as you wish without warning and as often as you like. That's totally fine.

The problem is, that this is not clearly stated anywhere and people started integrating this version of libmp4v2 into public repositories. The most prominent example is probably Arch Linux which ships with this version of libmp4v2.

It would be great if you could add a clear statement to your Readme.md explaining that this version is for internal use at TechSmith and you will introduce breaking changes at your discretion. Maybe even add a warning to not use this fork in public software repositories.

[ryandesign]

MacPorts recently updated its mp4v2 port from the old 2.0.0 version to this TechSmith fork, unaware that it was not fully backward-compatible, and is now encountering this incompatibility in build failures of software that uses mp4v2.

This fork is the first result when searching GitHub or DuckDuckGo for "mp4v2". As such it is probably where many people will go when looking for this software. I agree with @enzo1982. I suggest that either this fork make itself compatible with historical mp4v2 once again, or else leave a prominent notice in the readme that this fork is not compatible with historical mp4v2 and/or that it is not intended to be used by the public.

The introduction of C++ features into the public headers does seem like a bug / oversight though that should be corrected. doc/MP4.3 still says:

The MP4 library can be used by either C or C++ programs. The calling convention is C,

[nefthy]

ShouldParseAtomCallback cb = nullptr

should be

ShouldParseAtomCallback cb DEFAULT(nullptr)

for C to work

[barracuda156]

Has this issue been fixed?

[gavtroy]

No but the usual distros have headed to enzo1982/mp4v2 (https://mp4v2.org/) which is more aligned with broad use.

[barracuda156]

@ryandesign Should we as well?