Bugzilla Bug 61528

This package is marked stable under amd64 despite the niggling little detail of
it not actually working.  It compiles and installs but cannot decode m4a's.

Reproducible: Always
Steps to Reproduce:
1. emerge /usr/portage/media-libs/faad2/faad2-2.0-r2.ebuild
2. Try and play an m4a in xmms
3. "!$^&*, it doesn't work!"

Actual Results:  
It cannot play m4a's, giving the error anyone who's ever tried faad2 under
amd64
recognizes and loathes:  "MP4:  Pulse coding not allowed in short blocks".

Expected Results:  
It ought to be able to understand m4a files.

Portage 2.0.50-r9 (default-amd64-2004.0, gcc-3.3.3, glibc-2.3.4.20040808-r0,
2.6.4-gentoo-r1)
=================================================================
System uname: 2.6.4-gentoo-r1 x86_64 5
Gentoo Base System version 1.5.1
Autoconf: sys-devel/autoconf-2.59-r4
Automake: sys-devel/automake-1.8.5-r1
ACCEPT_KEYWORDS="amd64 ~amd64"
AUTOCLEAN="yes"
CFLAGS="-O2 -pipe"
CHOST="x86_64-pc-linux-gnu"
COMPILER="gcc3"
CONFIG_PROTECT="/etc /usr/X11R6/lib/X11/xkb /usr/kde/2/share/config
/usr/kde/3.1/share/config /usr/kde/3/share/config
/usr/lib/mozilla/defaults/pref
/usr/share/config /var/qmail/control"
CONFIG_PROTECT_MASK="/etc/gconf /etc/terminfo /etc/env.d"
CXXFLAGS=""
DISTDIR="/usr/portage/distfiles"
FEATURES="autoaddcvs ccache"
GENTOO_MIRRORS="http://mirrors.tds.net/gentoo ftp://mirrors.tds.net/gentoo
ftp://ftp.ndlug.nd.edu/pub/gentoo/"
MAKEOPTS="-j3"
PKGDIR="/usr/portage/packages"
PORTAGE_TMPDIR="/var/tmp"
PORTDIR="/usr/portage"
PORTDIR_OVERLAY="/usr/local/portage/"
SYNC="rsync://rsync.gentoo.org/gentoo-portage"
USE="X aalib alsa amd64 apm arts avi berkdb cdr crypt cups dvd encode esd
foomaticdb gdbm ggi gif gnome gpm gtk gtk2 guile imlib jpeg kde libg++ libwww
mikmod motif mozilla mpeg multilib mysql ncurses nls nogcj oggvorbis opengl oss
pam pdflib perl png ppds python qt quicktime readline scanner sdl slang spell
ssl tcltk tcpd truetype usb xml2 xmms xv zlib"

It's been a month, and faad2 is still broken.  Call me a purist, but I don't
think well-known borked packages deserve to be flagged 'stable' just because
they compile.

Yep..this problem still exists despite the patches
this is the original now closed bug
http://bugs.gentoo.org/show_bug.cgi?id=58780

It's a whole new year, and this broken package is still marked as "stable".  I
admit it does not crash, but it is completely useless under amd64 for it's
stated function of decoding MPEG-4 and MPEG-2 audio files.

well, it's also used to make mplayer play mpeg4 video files, and that works
fine. could you provide an example m4a file so we can test it? i don't have
such a file

Here's a sample audio file that doesn't play properly: 
http://burningsmell.org/crash_powermac_lc.m4a

mp4 video seemingly works, but mp4 audio is broken.  Most of the time they're
rendered as pure silence, sometimes clicks, and once in a blue moon mplayer
plays them properly, but xmms never works, and this audio file in particular
doesn't work under either.

I can confirm that the above file will decode properly with an x86 compiled
version of faad2. 

It also will NOT decode properly with a amd64 compiled version of faad2. It
will the produce the same 

"Error: Pulse coding not allowed in short blocks
Error: Pulse coding not allowed in short blocks
Error: Pulse coding not allowed in short blocks
Segmentation fault"
that has been refrenced before. 

just did some more scrounging....the latest CVS of faad2 is fixed and decodes
perfectly for amd64 
the xmms plugin however does not build 

Finally got to the root of this obnoxious bug!

It seems faad2 has two different headers.  There's the ones it uses internally, which are strictly defined as 32-bit everything;  then there's the one it installs in the system which have longs all over the place, which of course become 64-bit under amd64.

So faad2 itself was working, but anything linking to faad2 would call these 32-bit functions with 64-bit types. This led to all kinds of miscellaneous hilarity, like the XMMS plugin failing with faad's 'pulse coding not allowed in short blocks' error code when faad was actually returning success!  It also had some ugliness where it didn't define headers for some functions and didn't #include <string.h> -- which of course the linker can fix for the programmer under a 32-bit system, but breaks horribly under 64-bit.

After posting this I'll attach a patch that applies directly to faad2-2.0.tar.gz.  It allows faad2 to compile and work(as long as you fix the broken makefile ./bootstrap and ./configure produce, but I think that's covered by other patches).  It's been able to play every mp4 on my system flawlessly, and has been 100% stable so far.

Created an attachment (id=50096) [details]
amd64 patch for faad2's inconsistent headers and misc nastiness

This patch works for me.
Thanks

created a combined patch from the noext and your patch, otherwise it didn't
work. i've added this patch to the tree, -r5 applies it. thanks for your
effort.

i'm trying to use faad2 on x86 and these amd64 changes have changed the API in
an incompatible way. mythmusic will not compile against it:

aacdecoder.cpp: In member function `bool aacDecoder::initializeMP4()':
aacdecoder.cpp:298: error: invalid conversion from `long unsigned int*' to `
   unsigned int*'

can these changes me made specific to the amd64?

They are legit also on x86, the changes needs to be done upstream and the apps
which relies on the old behaviour should be fixed.

reopening, other programs which rely on the old behaviour dont like the patch.
removing...

We NEED that patch, or something like it.  Things will not work without it
under
amd64 because the old behavior is flagrantly wrong.  I'll try and make a less
bothersome patch but some breakage might be inevitable -- it was always broken,
the compiler just didn't know it.

the problem is that upstream has to accept these changes, i'll keep bugging
them. i've send the patch upstream and commited it on the same day. other
projects which rely up on the old (bad or broken) behaviour have to change
their programs, too.

Was there a change in policy recently?  It's a hell of a lot harder to get 
and keep fixes in portage than it used to be.  A *year* without any official
releases is more than enough justification for including our own patches imho.

Anyway I hope you'll like this one better, since it doesn't break compilation
of anything -- the installed header is not modified.  The patch and ebuild
(which I'll attach shortly) build a working xmms plugin etc, and apply all
the r4 patches as well as the amd64 one.  I've tested it in my portage overlay.

Any linking on vanilla x86 wouldn't break since there's no effective difference
between an int and a long there.  Anyone that built r5 on amd64 would need to
rebuild things that linked to faad unfortunately.

it's wasnt taken out of the tree because of a policy. the problem is that the
patch changes the api, even though this change is needed and sane. other
programs, atleast 2 which are actually in the tree break due to this patch.
I've removed it temporaly until i get the change to talk to upstream. foser, or
better another user who talked to foser, pointed me to this fact. it really
seems that upstream is dead or inactive, something has to be done. i will try
to get this issue fixed next week.

Created an attachment (id=58217) [details]
improved amd64 faad2 patch

Modifies faad's internal headers instead of the external ones so as to not
break compilation of anything depending on faad.

Created an attachment (id=58218) [details]
ebuild using amd64 patch

That patch is bogus. The redefinition of uintX_t isn't working as the logic is
wrong, and anyway stdint.h takes care of defining them correctly depeding on
arch.

Removing references to standard ints is not good at all. They are used to avoid
size misunderstanding.

That can't be applied, sorry.

The right way is to change the public api and fix the apps accordling. Avidemux
is already fixed, both mplayer and xine-lib uses internal copies.

> That patch is bogus. The redefinition of uintX_t isn't working as the logic
> is wrong, and anyway stdint.h takes care of defining them correctly depeding
> on arch.
If the logic is wrong, how about telling me *how* it's wrong so I don't make
that mistake again?  Works perfectly for me.  I know perfectly well those types
won't be used unless it can't find stdint.h but I put them there to be
thorough.  As for stdint.h "properly defining the integer types", it sure does,
but faad2 fails to use them for the external library interface!  Lot of good
that does.

> Removing references to standard ints is not good at all. They are used to
> avoid size misunderstanding.
HAHAHAHAHAHA! XD  'avoid size misunderstanding'.  That's what you call a year+
of muddling around with "Pulse coding not allowed in short blocks" errors
when faad2 was actually returning SUCCESS but the interfaces were so mangled
we never knew it.  The actual bits that NEED specific integer sizes are all
internal, confusion is caused by USING those types internally because the
external header does NOT.  Anyway we already tried that solution with R5 and it
was summarily rejected.

Granted the way faad2 handles these interfaces is ugly -- why can't it just
use the same damn header for both? :( -- but unless you feel like completely
rewriting faad to unify the internal and external headers, we have to work with
what we have.

Let me suggest a compromise:

Bring back R5, or R7 now I guess, but *mask it* for anything but amd64 and
ppc64.  I'll submit a improved improved patch for it that uses the holy standard
integer types.  We can likewise mask fixes for for whatever R5 breaks,
additionally adding a >=faad-2.0r7 dependency in them.  Then once we've got
most everything working with R7, THEN we unmask all those for x86 and the like.
amd64ers get what we want, a working faad2.  x86ers get what they want, a
working faad2 and working libs with it.

We'll have to work quickly so we don't block new versions of things that use
faad, but I am completely willing to *personally* do all the patchwork fixes if
anyone will tell me what things R5 broke.

Created an attachment (id=58582) [details]
improved improved amd64 faad2 patch

Uses standard integer types in faad.h to match faad's internal headers.
Replacement for previous patch, works properly with ebuild above

This patch seems ok to me.. I tested it on amd64 and x86 .. but I think its
only necessary to play around with the long... bug I guess the others can't
hurt (until the next version).

alright, no one complains so I put it back in (into -r6)... blame me if it
breaks again

this patch got removed in -r7 because it breaks (and putting the patch in -r6
without revbump caused even more headaches), fresh suggestions would be appreciated.

(removing myself from cc, I already receive this through amd64@g.o) 
 
The most of headaches is when the patch is added and then removed and then  
readded. 
I think the patch currently in -r7 (and added in -r6) is good enough, *but* we  
must stop adding and removing it and just fix the broken apps. I can fix the  
ones in video/sound, but we must find a stable API for the next versions. 
 
By the way, currently upstream seams quite dead or at least doesn't care too  
much from users; xine-lib devs seems also in trouble with its license. So 
whichever decision we'll make, should be quite safe for now that no new faad2 
version will come up. 
 

Which apps does the current patch break? I'll see if I can fix them..

it breaks the fadd plugin in gstreamer, I don't se how this can bee fixed
without a upstream release of fadd2. The gst-plugin can't be changed as then it
won't build elsewhere.

I'm not following you.  Where won't it build if we don't leave it broken?

Chalk up media-video/vlc as another package that doesn't seem to like the 
patched faad2.  When playing an h264/aac mp4 file, the video plays properly, 
but the sound comes out sounding with a horrible static sound. 

ok, this has been open for quite some time. faad2 is broken on amd64 and needs
fixing to work correctly. taking the patch in and out doesn't solve the issue.
upstream seems to be dead and we have devs willing to patch packages using faad2
to work with the changed api.

plan: put in a new package.mask'ed revision of faad2 including the patch and
have all current ebuilds using the old faad2 api depend on below this new
revision. then our devs can start to patch packages and have them depend on at
least the fixed revision of faad2. if you commit them to the tree, they need
to be package.mask'ed too to not cause broken deps. if you unmask the new faad2
ebuild please make sure that every package using it also gets unmasked and that
a new patched revision is available, else portage might do the upgrade and
downgrade thing because of dependencies not sharing common matches. add a
postinst message to the new faad2 ebuilds that the api has changed and that
users need to update all packages that use faad2.

how does that sound?

faad2 took over by sound herd, the patch is in place, I think the problem is
solved now.