Breaking API change in 1.29.9 — intentional or not?

[zmwangx]

12d7414f7ece191783e54d92beb46e0e182202f6 introduced a breaking API change, config.allowMidside => config. jointmode that broke compatibility with current Libav (https://code.videolan.org/libav/libav/blob/master/libavcodec/libfaac.c#L118) and maintenance lines of FFmpeg, e.g. 2.8.13 (FFmpeg dropped faac support in 3.2). While Libav could and probably should be updated to reflect this change, it's probably hard to fix this in the maintenance lines of FFmpeg.

I package FFmpeg and related packages for the Homebrew package manager, so I just want to make sure this change is intentional and final before dropping faac support from our FFmpeg 2.x package. Thanks for your attention.

[zmwangx]

(To be more clear, I was wondering whether a compatibility interface is possible.)

[knik0]

A simple patch would fix it: don't set allowMidside nor jointmode, default values are set in the library. You probably only need to set bitrate, no need to mess with advanced settings.

[ilovezfs]

@knik0 would it be possible to make faacConfig->allowMidside = 1 a no-op somehow rather than a build failure? This broke every use of faac in Homebrew, and so we'll either need to chase every upstream for those packages around to patch this, or drop faac support from every package using it.

[knik0]

You mean something like adding unused allowMidside field to the struct? That might do the trick.

[ilovezfs]

Yep, something that accounts for the fact that consumers of faac seem to have treated that field as public API and been fond of explicitly setting it.

Note I just saw https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=223416

[knik0]

I see, people prefer broken API over updated/improved API. That's not very optimistic. Has anyone even noticed there is FAAC_CFG_VERSION macro and a version (config version) field in the config struct? I guess not.

[ilovezfs]

@knik0 I don't have a preference one way or the other, but if all of the patches are just going to be

- aacConfig->allowMidside = 1
+ // aacConfig->allowMidside = 1

Then it seems like we might well save everyone the trouble.

[ilovezfs]

@knik0 if someone has

aacConfig->allowMidside = 0

what would be the correct "new" way of setting that?

[knik0]

Just remove all "aacConfig->allowMidside" in the code.

All defaults are set in libfaac, you only set the values you really need, e.g. aacConfig->bitRate (that one could be useful), that's it.

[ilovezfs]

Yeah if the outcome is going to be all aacConfig->allowMidside = 0 get removed && all aacConfig->allowMidside = 1 also get removed, then an unused variable seems like a reasonable approach to avoid the breaking change.

[knik0]

Then other folks may start complaining about allowMidside not working. Please tell me how many times faacConfig->allowMidside is set in all the existing code, I bet that would be less than 10. Is it really so hard to fix it?

[ilovezfs]

The difficulty is not in fixing it, but in getting the upstreams for these packages to merge the fixes. If there's no hope of getting the fixes merged upstream, we remove the functionality as opposed to applying permanent downstream patches.

[ilovezfs]

Concretely,

darkice.rb:20:  depends_on "faac"
ffmbc.rb:25:  depends_on "faac" => :recommended
ffmpeg@2.8.rb:50:  depends_on "faac" => :optional
gst-plugins-bad.rb:28:  depends_on "faac" => :optional
gst-plugins-bad@0.10.rb:29:  depends_on "faac" => :optional
libav.rb:38:  depends_on "faac" => :recommended

So "someone" will need to open pull requests with fixes to each of those projects and explain this change to them.

[ilovezfs]

In older versions of faac that still have the allowMidside field, is the default 1 if it's unset? Or does it have to be set? The first question projects are going to ask is whether simply removing aacConfig->allowMidside = 1 is a backwards compatible change.

[knik0]

The first question projects are going to ask is whether simply removing aacConfig->allowMidside = 1 is a backwards compatible change

Yep, it is. It was like that for years, no problem whatsoever.

BTW. Maybe you could use faac 1.29.8.3 with those packages -- full compatibility.

[enzo1982]

Simply replacing aacConfig->allowMidside = X with aacConfig->jointmode = X should work too and would keep functionality of the affected packages.

As far as I understand it, allowMidside = 0 is equal to jointmode = 0 (JOINT_NONE) and allowMidside = 1 is equal to jointmode = 1 (JOINT_MS). That's how I did it with fre:ac for now anyway, until I'll add support for JOINT_IS mode.

@knik0 has a good point in that adding a dummy allowMidside would silently break functionality of existing software which I think would be worse than forcing a minimal change.

[ilovezfs]

BTW. Maybe you could use faac 1.29.8.3 with those packages -- full compatibility.

We'd only be able to do that if you're planning on maintaining a separate 1.28.x (not 29) going forward.

[ilovezfs]

As far as I understand it, allowMidside = 0 is equal to jointmode = 0 (JOINT_NONE) and allowMidside = 1 is equal to jointmode = 1 (JOINT_MS). That's how I did it with fre:ac for now anyway, until I'll add support for JOINT_IS mode.

If it's really one-to-one like that, why not add a compatibility interface?

[enzo1982]

Maybe make a union out of int allowMidside and int jointmode in faacEncConfiguration? That should work around all the issues while keeping source and binary compatibility.

[knik0]

@enzo1982 Man, you're genius.

Maybe make a union out of int allowMidside and int jointmode in faacEncConfiguration? That should work around all the issues while keeping source and binary compatibility.

[ilovezfs]

@knik0 I think this can be closed now :)