Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#22 Correct FLAC inline and fix compiling with Sun Studio

open-fixed
Josh Coalson
None
8
2009-01-03
2007-04-17
Brian Cameron
No

This patch fixes a number of problems with FLAC. Note this bug is a new patch that works with FLAC 1.4.0 and replaces bug 1214424. I couldn't figure out how to update that bug with a new patch.

1) Mainly it fixes flac so it properly tests for inline
in the configure script and consistently uses
FLAC_INLINE everywhere. This properly sets inline
to work on different compilers by using AC_C_INLINE.
Note you need to run autoheader and autoconf after
applying the patch for config.h to have the define
FLAC_INLINE.

2) Fix src/share/replaygain_analysis so it compiles
with Sun Studio. Sun Studio doesn't like floating
points in array values. So I fixed this by changing
RMS_WINDOW_TIME from 0.050 to 50 (so it is specified
in ms rather than s). Then in the two places where
it is referenced I add " / 1000" so it gives the
same value. I think this is cleaner. Plus it works
with more compilers.

3) Note that it was necessary to redefine four things
as extern instead of static. This is because
Sun Studio does not allow you to reference static
elements from within extern inline functions.

bitreader_read_from_client and byte_to_unary_table
in src/libFLAC/bitreader.c
bitwriter_grow_ in src/libFLAC/bitwriter.c
is_big_endian_host_ in src/libFLAC/md5.c.

This was necessary because Sun Studio doesn't
allow you to reference static variables from
extern inline functions.

Also byteSwap in src/libFLAC/md5.c needed to be
more clearly defined as "extern" to make the
linker happy.

4) Sun Studio doesn't like the ^M characters at the
end of each line in
include/share/replaygain_analysis.h, so the
patch removes these.

Discussion

  • Brian Cameron
    Brian Cameron
    2007-04-17

    patch fixing FLAC

     
    Attachments
  • Josh Coalson
    Josh Coalson
    2007-04-21

    • priority: 5 --> 6
    • assigned_to: nobody --> jcoalson
    • status: open --> open-accepted
     
  • Brian Cameron
    Brian Cameron
    2007-05-01

    Logged In: YES
    user_id=689771
    Originator: YES

    Can this go upstream? I think this patch makes FLAC much more portable and shouldn't contain anything controversial.

     
  • Josh Coalson
    Josh Coalson
    2007-07-26

    Logged In: YES
    user_id=78173
    Originator: NO

    I'm a little bit wary about applying this, the reason I had FLaC__INLINE before was because of an autoconf bug that 'owned' any symbol that had AC_ anywhere in it and caused all kinds of problems... let me take a look more closely

     
  • Logged In: NO

    Seems to work okay for me as FLAC_INLINE, though changing this string to something else that avoids the "AC_" should be a trivial change. Do you need me to change this, or do you want to? This is Brian.Cameron@sun.com (user yippi).

     
  • Josh Coalson
    Josh Coalson
    2007-07-31

    Logged In: YES
    user_id=78173
    Originator: NO

    I'll do it

     
  • Brian Cameron
    Brian Cameron
    2007-08-01

    updated patch to fix this issue.

     
    Attachments
  • Brian Cameron
    Brian Cameron
    2007-08-01

    Logged In: YES
    user_id=689771
    Originator: YES

    Here is an updated patch that applies cleanly against 1.4.0.

    This patch addresses the 4 above issues.

    Note that in 1.4.0 you did do some work to fix issue #2. However,
    I don't think that fix is so great. It's not good to hardcode the
    value only on Sun.

    So I changed this again so that RMS_WINDOW_TIME is changed from 0.050 to 50 (so it is specified in ms rather than s). Then in the
    two places where it is referenced I add " / 1000". This way it gives
    the same value. This compiles fine with Sun Studio. Sun Studio just doesn't like the numeric values with the decimal points (it likes 1 but doesn't like 1.").

    If you really want to keep the "." characters in there when compiling with GCC, then perhaps you should "#ifdef __GNUC__" and in the else use the code without the ".". Note that #ifdef __sun isn't so good because users can use the GCC compiler on Sun. Though I'd still recommend changing RMS_WINDOW_TIME to 50 and its usage to "RMS_WINDOW_TIME / 1000" just because it's nicer.

    I think this is a cleaner fix for issues #2, since it doesn't hardcode the value when using Sun Studio.

    I know you are talking about changing the AC_INLINE to a different string, but I'll let you do that since you said you would. However AC_INLINE works fine with the latest autotools on Solaris. Perhaps this is a bug with older versions of the autotools that we don't need to support anymore?
    File Added: flac-01-forte.diff

     
  • Jan Engelhardt
    Jan Engelhardt
    2007-08-24

    Logged In: YES
    user_id=1287009
    Originator: NO

    Can't you use -Dinline=__inline__, if the compiler does not know about 'inline', instead of using shouting macros like FLAC_INLINE?

    e.g. configure.ac:
    AC_C_INLINE
    if [ "$ac_cv_c_inline" != no ]; then
    AC_CFLAGS="$AC_CFLAGS -Dinline=$ac_cv_c_inline";
    fi;
    ...
    AC_SUBST(AC_CFLAGS)

    Makefile.am:
    AM_CFLAGS = $(AC_CFLAGS) $(libogg_CFLAGS) $(etc_CFLAGS)

     
  • Brian Cameron
    Brian Cameron
    2007-11-06

    Logged In: YES
    user_id=689771
    Originator: YES

    Can this patch go upstream? It's been a long time since this bug has been updated. If you want me to change the patch to use some other string than FLAC_INLINE, I can change that if you want. It would be nice if FLAC had better inline support.

     
  • Josh Coalson
    Josh Coalson
    2007-11-07

    Logged In: YES
    user_id=78173
    Originator: NO

    it will eventually get in... I haven't had a lot of free time to work on FLAC lately

     
  • Brian Cameron
    Brian Cameron
    2008-12-08

    *ping* Can this go upstream? This bug report seems to have languished for over a year.

     
  • Josh Coalson
    Josh Coalson
    2009-01-01

    • priority: 6 --> 8
     
  • Josh Coalson
    Josh Coalson
    2009-01-03

    • status: open-accepted --> open-fixed
     
  • Josh Coalson
    Josh Coalson
    2009-01-03

    ok, I checked in fixes for everything I think. can you try with cvs head?