Re: [PATCH] winemp3.acm: always dynamically load libmpg123 stuff

[Chris Robinson]

On Sunday 28 February 2010 1:13:44 pm Marcus Meissner wrote:
> Hi,
>
> Copy in the mpg123.h header from the library so we do not need it for
> building. (it is also LPGL)
>
> Dynamically link from libmpg123.so.0.
> (Chances are very good that ABI stays stable.)
>
> This allows me providing a MP3 capable Wine build for openSUSE without
> actually having libmpg123 in the openSUSE buildservice for building.
>
> I was not able to test it though, anyone have a working winemp3.acm
> using testcase :(

This won't work, unfortunately. The mpg123.h header does some evil macro hacks 
to forward certain functions (eg. mpg123_seek, mpg123_feedseek) to 64-bit 
variants when _FILE_OFFSET_BITS is 64 due to using off_t. The functions the 
lib has depends on whether it was built with _FILE_OFFSET_BITS=64 or not, and 
the macros won't affect the strings passed to wine_dlsym.

It also looks like the lib isn't unloaded with the acm.

[PATCH] winemp3.acm: always dynamically load libmpg123 stuff

[Marcus Meissner]

On Sun, Feb 28, 2010 at 03:11:22PM -0800, Chris Robinson wrote:
> On Sunday 28 February 2010 1:13:44 pm Marcus Meissner wrote:
> > Hi,
> >
> > Copy in the mpg123.h header from the library so we do not need it for
> > building. (it is also LPGL)
> >
> > Dynamically link from libmpg123.so.0.
> > (Chances are very good that ABI stays stable.)
> >
> > This allows me providing a MP3 capable Wine build for openSUSE without
> > actually having libmpg123 in the openSUSE buildservice for building.
> >
> > I was not able to test it though, anyone have a working winemp3.acm
> > using testcase :(
>
> This won't work, unfortunately. The mpg123.h header does some evil macro hacks 
> to forward certain functions (eg. mpg123_seek, mpg123_feedseek) to 64-bit 
> variants when _FILE_OFFSET_BITS is 64 due to using off_t. The functions the 
> lib has depends on whether it was built with _FILE_OFFSET_BITS=64 or not, and 
> the macros won't affect the strings passed to wine_dlsym.

A library should be ever built against one model and not change function ABI
depending on a preprocessor define ... This will just break in funny ways.

In libmpg123 1.10.0 it does not do evil hacks for me, it has #if 0 around
these parts (see the header included in the patch). Seems to have been an earlier
try.

Is this different in newer / other versions? Then I have to think about something :(
> It also looks like the lib isn't unloaded with the acm.

Yeah, need to fix this.

Ciao, Marcus

[PATCH] winemp3.acm: always dynamically load libmpg123 stuff

[Chris Robinson]

On Sunday 28 February 2010 11:23:30 pm Marcus Meissner wrote:
> A library should be ever built against one model and not change function
> ABI depending on a preprocessor define ... This will just break in funny
> ways.
>
> In libmpg123 1.10.0 it does not do evil hacks for me, it has #if 0 around
> these parts (see the header included in the patch). Seems to have been an
> earlier try.

I believe it puts #if 0 when the lib is built without _FILE_OFFSET_BITS=64. In 
my mpg123.h (which is built with _FILE_OFFSET_BITS=64), it has #if 1.

Seems newer versions/trunk just takes the base function name and appends the 
_FILE_OFFSET_BITS value, which still causes ABI issues
http://mpg123.de/cgi-bin/viewvc.cgi/trunk/src/libmpg123/mpg123.h.in

That's the remaining thing that bugs me about mpg123, how it has a different 
ABI with the same lib version depending on its build options. libFLAC and 
libvorbisfile, at least, have the sense to use a 64-bit offset type 
explicitly, instead of using off_t.