mp3 update

[Aric Stewart]

Hello all,

I am working on updating our winemp3.acm implementation to a modern 
libmpg123. We where at about 0.59r  and I have 1.8.1 working very well.

I have a few questions.  Right now I have the smallest subset of files 
from libmpg123 that are needed to compile and work in the wine framework 
  (mostly) unmodified. I have replaced all the malloc/free calls with 
HeapAlloc/HeapFree and worked in all the wine debugging code instead of 
the libmpg123 debugging code.

What I am wondering about is that there is still a lot of code in these 
files we have no interest in.  The file reader and http reader and other 
things that we do not use (it is accessed just by the direct stream 
methods) and those things dependencies.

Should I leave all this code present in our version of the modules and 
mostly preserve the libmpg123 work to make it easier to keep updating 
forward?  Or should i remove all the unneeded code so that the modules 
present in winehq are leaner and more targeted to just the things the 
wine needs, but then they deviate much more from the original libmpg123 
source?

With what I have now the mp3 source material this game i am testing is 
using sounds much much better. So it should be a great improvement.

Thanks,
-aric

[Juan Lang]

Hi Aric,
> What I am wondering about is that there is still a lot of code in these
> files we have no interest in.  The file reader and http reader and other
> things that we do not use (it is accessed just by the direct stream methods)
> and those things dependencies.
>
> Should I leave all this code present in our version of the modules and
> mostly preserve the libmpg123 work to make it easier to keep updating
> forward?  Or should i remove all the unneeded code so that the modules
> present in winehq are leaner and more targeted to just the things the wine
> needs, but then they deviate much more from the original libmpg123 source?

Personally, I don't see the point in adding files to the build that
aren't used.  So if an entire file isn't referenced from Wine, I
wouldn't add it in the first place.

If there's a file that is used in Wine, but that has code that isn't
used, you might add it unmodified as one patch, and remove the unused
code in another.  The purpose for adding it unmodified is to make it
easier to diff with the upstream version.

Another approach would be to add the unmodified code, then disable the
unused code using comments or #if 0.  This could be useful if we were
trying to apply patches to upstream to our own codebase.  It seems as
though we update from upstream sources rather seldom, though, so I
don't think this is worth the ugliness of dead code being left in the
source.

A final complication is when one file, which is used, contains
references to files that aren't used.  In this case I'd say add the
file with the references removed, and hopefully the deletions would be
clear in the diff from upstream.

This is all my opinion of course.  What were you thinking?
> With what I have now the mp3 source material this game i am testing is using
> sounds much much better. So it should be a great improvement.

Well done :)
--Juan

[Chris Robinson]

On Wednesday 12 August 2009 1:10:15 pm Aric Stewart wrote:
> I have a few questions.  Right now I have the smallest subset of files
> from libmpg123 that are needed to compile and work in the wine framework
>   (mostly) unmodified. I have replaced all the malloc/free calls with
> HeapAlloc/HeapFree and worked in all the wine debugging code instead of
> the libmpg123 debugging code.
>
> What I am wondering about is that there is still a lot of code in these
> files we have no interest in.  The file reader and http reader and other
> things that we do not use (it is accessed just by the direct stream
> methods) and those things dependencies.
>
> Should I leave all this code present in our version of the modules and
> mostly preserve the libmpg123 work to make it easier to keep updating
> forward?  Or should i remove all the unneeded code so that the modules
> present in winehq are leaner and more targeted to just the things the
> wine needs, but then they deviate much more from the original libmpg123
> source?
>
> With what I have now the mp3 source material this game i am testing is
> using sounds much much better. So it should be a great improvement.

Personally, I'd think linking libmpg123 would be a better option, instead of 
duplicating it. It would allow Wine to be updated when the lib updates, 
instead of trying to keep up with changes, and avoid duplicating 
functionality.
> Thanks,
> -aric

[Steven Edwards]

On Wed, Aug 12, 2009 at 4:34 PM, Chris Robinson wrote:
> Personally, I'd think linking libmpg123 would be a better option, instead of
> duplicating it. It would allow Wine to be updated when the lib updates,
> instead of trying to keep up with changes, and avoid duplicating
> functionality.

Right. Is there an particular reason we can't dlopen the native library?

Thanks

[Juan Lang]

> Right. Is there an particular reason we can't dlopen the native library?

I don't think dlopen is preferable, quite the opposite in fact.
dlopen leads to a situation in which Wine has features that work on
the build system, but not at runtime on another system that doesn't
have the library available.  Statically linking to our dependencies
avoids this, but it's only possible when the license is compatible.  I
think that's why we use dlopen, e.g. for OpenSSL, where due to license
restrictions we can't statically link the code.
--Juan

[Austin English]

On Wed, Aug 12, 2009 at 4:09 PM, Juan Lang wrote:
> > Right. Is there an particular reason we can't dlopen the native library?
>
> I don't think dlopen is preferable, quite the opposite in fact.
> dlopen leads to a situation in which Wine has features that work on
> the build system, but not at runtime on another system that doesn't
> have the library available.  Statically linking to our dependencies
> avoids this, but it's only possible when the license is compatible.  I
> think that's why we use dlopen, e.g. for OpenSSL, where due to license
> restrictions we can't statically link the code.
> --Juan

If the code can be copied into wine itself, it seems to reason that we
could statically link that same code ;-).

http://www.mpg123.org/ - it's LGPL 2.1.

[Juan Lang]

> If the code can be copied into wine itself, it seems to reason that we
> could statically link that same code ;-).
>
> http://www.mpg123.org/ - it's LGPL 2.1.

Oh yes, I assumed so.  I was talking about statically linking vs.
dlopen in general, and why avoiding dlopen where practical is a good
thing.  It's not always practical even when the license does agree,
for example for mshtml:  importing the Mozilla source tree is just too
much effort.

I also assume that, in the specific case of mpg123, there's some
technical reason we need to modify the source, and Aric was asking the
best way to do that.  Naturally this precludes statically linking and
dlopen.
--Juan

[Aric Stewart]

Well the main advantage I can see is that we are able to have mp3 
support without adding a new library dependency.  This will be 
especially useful for platforms other than Linux where libmpg123 is not 
present. Such as the Mac.

There is no technical or licensing reason we would have to link to the 
native library.

We could make a configure parameter that either links native existing 
libmpg123.so or builds the independent builtin version.

-aric

Steven Edwards wrote:
> On Wed, Aug 12, 2009 at 4:34 PM, Chris Robinson wrote:
> > Personally, I'd think linking libmpg123 would be a better option, instead of
> > duplicating it. It would allow Wine to be updated when the lib updates,
> > instead of trying to keep up with changes, and avoid duplicating
> > functionality.
>
> Right. Is there an particular reason we can't dlopen the native library?
>
> Thanks

[Steven Edwards]

On Wed, Aug 12, 2009 at 6:30 PM, Aric Stewart wrote:
> Well the main advantage I can see is that we are able to have mp3 support
> without adding a new library dependency.  This will be especially useful for
> platforms other than Linux where libmpg123 is not present. Such as the Mac.

Heh yeah I didn't think about this.
> There is no technical or licensing reason we would have to link to the
> native library.
>
> We could make a configure parameter that either links native existing
> libmpg123.so or builds the independent builtin version.

It's not worth the trouble. In fact what your doing sounds perfect now
that I've had a chance to think about it. We already pull in too much
crap for external libraries.

[Stefan Dösinger]

Am Thursday 13 August 2009 00:30:33 schrieb Aric Stewart:
> Well the main advantage I can see is that we are able to have mp3
> support without adding a new library dependency.  This will be
> especially useful for platforms other than Linux where libmpg123 is not
> present. Such as the Mac.

How hard is it to make a configure option whether to link to libmpg123 
dynamically or statically, and probably default to dynamic linking(not 
dlopen).

That way package providers can build a package that doesn't need extra 
dependencies, self-compilers(where build system == runtime system) don't need 
two copies of the library on their HD, and distribution providers can decide 
what to do with their own wine package.

[Juan Lang]

> How hard is it to make a configure option whether to link to libmpg123
> dynamically or statically, and probably default to dynamic linking(not
> dlopen).
>
> That way package providers can build a package that doesn't need extra
> dependencies, self-compilers(where build system == runtime system) don't need
> two copies of the library on their HD, and distribution providers can decide
> what to do with their own wine package.

I don't get the benefit.  Disk space, in this day and age?  Developer
resources are a much bigger constraint.  Please, no additional
configure options if we don't need them.
--Juan

[Chris Robinson]

On Thursday 13 August 2009 11:23:45 am Juan Lang wrote:
> Developer resources are a much bigger constraint.

All the more reason to use the existing libmpg123 instead of trying to 
maintain our own version, IMO.

[Juan Lang]

> All the more reason to use the existing libmpg123 instead of trying to
> maintain our own version, IMO.

Aric already explained that we can't:  it doesn't exist on the Mac.
--Juan

[Austin English]

On Thu, Aug 13, 2009 at 1:41 PM, Juan Lang wrote:
> > All the more reason to use the existing libmpg123 instead of trying to
> > maintain our own version, IMO.
>
> Aric already explained that we can't:  it doesn't exist on the Mac.

Not to nitpick, but if that's the reason for bundling libmpg, there
are several other libraries we should bundle...

[Juan Lang]

> Not to nitpick, but if that's the reason for bundling libmpg, there
> are several other libraries we should bundle...

You won't get disagreement from me there, but now we're changing the subject.
--Juan

[Alexandre Julliard]

Austin English  writes:
> On Thu, Aug 13, 2009 at 1:41 PM, Juan Lang wrote:
> > > All the more reason to use the existing libmpg123 instead of trying to
> > > maintain our own version, IMO.
> >
> > Aric already explained that we can't:  it doesn't exist on the Mac.
>
> Not to nitpick, but if that's the reason for bundling libmpg, there
> are several other libraries we should bundle...

It's not the reason, as you point out the Mac is lacking a lot of
things. I believe the reason we copied it was that it wasn't widely
available on Linux either, because of licensing issues. If it's now
included in the major distros then we should certainly consider dynamic
loading.

[Reece Dunn]

2009/8/13 Alexandre Julliard :
> Austin English  writes:
> > On Thu, Aug 13, 2009 at 1:41 PM, Juan Lang wrote:
> > > > All the more reason to use the existing libmpg123 instead of trying to
> > > > maintain our own version, IMO.
> > >
> > > Aric already explained that we can't:  it doesn't exist on the Mac.
> >
> > Not to nitpick, but if that's the reason for bundling libmpg, there
> > are several other libraries we should bundle...
>
> It's not the reason, as you point out the Mac is lacking a lot of
> things. I believe the reason we copied it was that it wasn't widely
> available on Linux either, because of licensing issues. If it's now
> included in the major distros then we should certainly consider dynamic
> loading.

The libmpg123-0 package was not present on my Ubuntu 9.04 box. The
licensing issues still apply for mp3 support.

If wine does want to go down this route, it would probably be better
to use the GStreamer interface rather than an mp3 library directly --
that way, it doesn't matter what mp3 library they are using, and also
supports them (for example) using the Ubuntu codecs pack or the
Fluendo codecs (not sure if these are the same thing).

A quick google search indicates that GStreamer is supported on the Mac.

- Reece

[Marcus Meissner]

On Thu, Aug 13, 2009 at 09:07:29PM +0200, Alexandre Julliard wrote:
> Austin English  writes:
> > On Thu, Aug 13, 2009 at 1:41 PM, Juan Lang wrote:
> > > > All the more reason to use the existing libmpg123 instead of trying to
> > > > maintain our own version, IMO.
> > >
> > > Aric already explained that we can't:  it doesn't exist on the Mac.
> >
> > Not to nitpick, but if that's the reason for bundling libmpg, there
> > are several other libraries we should bundle...
>
> It's not the reason, as you point out the Mac is lacking a lot of
> things. I believe the reason we copied it was that it wasn't widely
> available on Linux either, because of licensing issues. If it's now
> included in the major distros then we should certainly consider dynamic
> loading.

At least on SUSE I have to patch it out due to license issues, so my builds
do not have mp3 support.

Ciao, Marcus

[Alexandre Julliard]

Marcus Meissner  writes:
> On Thu, Aug 13, 2009 at 09:07:29PM +0200, Alexandre Julliard wrote:
> > Austin English  writes:
> > > On Thu, Aug 13, 2009 at 1:41 PM, Juan Lang wrote:
> > > > > All the more reason to use the existing libmpg123 instead of trying to
> > > > > maintain our own version, IMO.
> > > >
> > > > Aric already explained that we can't:  it doesn't exist on the Mac.
> > >
> > > Not to nitpick, but if that's the reason for bundling libmpg, there
> > > are several other libraries we should bundle...
> >
> > It's not the reason, as you point out the Mac is lacking a lot of
> > things. I believe the reason we copied it was that it wasn't widely
> > available on Linux either, because of licensing issues. If it's now
> > included in the major distros then we should certainly consider dynamic
> > loading.
>
> At least on SUSE I have to patch it out due to license issues, so my builds
> do not have mp3 support.

What issues?  The mp3 patents?  That's a different problem than the
original licensing issues, though of course it is also a good argument
for making it a dynamic dependency.

[Michael Stefaniuc]

Alexandre Julliard wrote:
> Marcus Meissner  writes:
> > On Thu, Aug 13, 2009 at 09:07:29PM +0200, Alexandre Julliard wrote:
> > > Austin English  writes:
> > > > On Thu, Aug 13, 2009 at 1:41 PM, Juan Lang wrote:
> > > > > > All the more reason to use the existing libmpg123 instead of trying to
> > > > > > maintain our own version, IMO.
> > > > >
> > > > > Aric already explained that we can't:  it doesn't exist on the Mac.
> > > >
> > > > Not to nitpick, but if that's the reason for bundling libmpg, there
> > > > are several other libraries we should bundle...
> > >
> > > It's not the reason, as you point out the Mac is lacking a lot of
> > > things. I believe the reason we copied it was that it wasn't widely
> > > available on Linux either, because of licensing issues. If it's now
> > > included in the major distros then we should certainly consider dynamic
> > > loading.
> >
> > At least on SUSE I have to patch it out due to license issues, so my builds
> > do not have mp3 support.
>
> What issues?  The mp3 patents?  That's a different problem than the

Yes. Fedora has the same problem.
> original licensing issues, though of course it is also a good argument
> for making it a dynamic dependency.

Right; saves the vendors the effort to patch it back out of Wine.

bye
    michael

[Marcus Meissner]

On Fri, Aug 14, 2009 at 12:52:48PM +0200, Michael Stefaniuc wrote:
> Alexandre Julliard wrote:
> > Marcus Meissner  writes:
> > > On Thu, Aug 13, 2009 at 09:07:29PM +0200, Alexandre Julliard wrote:
> > > > Austin English  writes:
> > > > > On Thu, Aug 13, 2009 at 1:41 PM, Juan Lang wrote:
> > > > > > > All the more reason to use the existing libmpg123 instead of trying to
> > > > > > > maintain our own version, IMO.
> > > > > > Aric already explained that we can't:  it doesn't exist on the Mac.
> > > > >
> > > > > Not to nitpick, but if that's the reason for bundling libmpg, there
> > > > > are several other libraries we should bundle...
> > > >
> > > > It's not the reason, as you point out the Mac is lacking a lot of
> > > > things. I believe the reason we copied it was that it wasn't widely
> > > > available on Linux either, because of licensing issues. If it's now
> > > > included in the major distros then we should certainly consider dynamic
> > > > loading.
> > >
> > > At least on SUSE I have to patch it out due to license issues, so my builds
> > > do not have mp3 support.
> >
> > What issues?  The mp3 patents?  That's a different problem than the
>
> Yes. Fedora has the same problem.
> > original licensing issues, though of course it is also a good argument
> > for making it a dynamic dependency.
>
> Right; saves the vendors the effort to patch it back out of Wine.

Just for the record,
I have no problem with patching it out, its just 1 commit in my local wine
git tree ;)

Ciao, Marcus

[Aric Stewart]

Sorry, I was out Friday and Monday so I am just getting back here.

Am i reading that the general opinion is that I should rework 
winemp3.acm to load the external libmpg123 and just abandon the builtin 
mp3 support?

Or was there still something else of issue with my work?

thanks,
-aric

Marcus Meissner wrote:
> On Fri, Aug 14, 2009 at 12:52:48PM +0200, Michael Stefaniuc wrote:
> > Alexandre Julliard wrote:
> > > Marcus Meissner  writes:
> > > > On Thu, Aug 13, 2009 at 09:07:29PM +0200, Alexandre Julliard wrote:
> > > > > Austin English  writes:
> > > > > > On Thu, Aug 13, 2009 at 1:41 PM, Juan Lang wrote:
> > > > > > >> All the more reason to use the existing libmpg123 instead of trying to
> > > > > > >> maintain our own version, IMO.
> > > > > > > Aric already explained that we can't:  it doesn't exist on the Mac.
> > > > > > Not to nitpick, but if that's the reason for bundling libmpg, there
> > > > > > are several other libraries we should bundle...
> > > > >
> > > > > It's not the reason, as you point out the Mac is lacking a lot of
> > > > > things. I believe the reason we copied it was that it wasn't widely
> > > > > available on Linux either, because of licensing issues. If it's now
> > > > > included in the major distros then we should certainly consider dynamic
> > > > > loading.
> > > >
> > > > At least on SUSE I have to patch it out due to license issues, so my builds
> > > > do not have mp3 support.
> > >
> > > What issues?  The mp3 patents?  That's a different problem than the
> >
> > Yes. Fedora has the same problem.
> > > original licensing issues, though of course it is also a good argument
> > > for making it a dynamic dependency.
> >
> > Right; saves the vendors the effort to patch it back out of Wine.
>
> Just for the record,
> I have no problem with patching it out, its just 1 commit in my local wine
> git tree ;)
>
> Ciao, Marcus

[Reece Dunn]

2009/8/13 Juan Lang :
> > How hard is it to make a configure option whether to link to libmpg123
> > dynamically or statically, and probably default to dynamic linking(not
> > dlopen).
> >
> > That way package providers can build a package that doesn't need extra
> > dependencies, self-compilers(where build system == runtime system) don't need
> > two copies of the library on their HD, and distribution providers can decide
> > what to do with their own wine package.
>
> I don't get the benefit.  Disk space, in this day and age?  Developer
> resources are a much bigger constraint.  Please, no additional
> configure options if we don't need them.

The reason you'd want to use dynamic linking is to ease security fix
updates. If a flaw is found in libmpg123 that allows remote code
execution (for example), any package that has its own version, or that
statically links it into the program, needs updating, rebuilding and
repackaging.

- Reece

[Juan Lang]

> The reason you'd want to use dynamic linking is to ease security fix
> updates. If a flaw is found in libmpg123 that allows remote code
> execution (for example), any package that has its own version, or that
> statically links it into the program, needs updating, rebuilding and
> repackaging.

Again, at what cost?  We have a patch proposed that fixes a real flaw
(mp3 sounds bad in Wine.)  You all are asking Aric to do more do
address flaws that are inconsequential, in my opinion (it takes more
disk space than it needs to) or only theoretical (the new code might
contain as yet unknown vulnerabilities.)

As always, patches talk louder than emails.
--Juan

[Reece Dunn]

2009/8/13 Juan Lang :
> > The reason you'd want to use dynamic linking is to ease security fix
> > updates. If a flaw is found in libmpg123 that allows remote code
> > execution (for example), any package that has its own version, or that
> > statically links it into the program, needs updating, rebuilding and
> > repackaging.
>
> Again, at what cost?  We have a patch proposed that fixes a real flaw
> (mp3 sounds bad in Wine.)  You all are asking Aric to do more do
> address flaws that are inconsequential, in my opinion (it takes more
> disk space than it needs to) or only theoretical (the new code might
> contain as yet unknown vulnerabilities.)
>
> As always, patches talk louder than emails.

I was not suggesting that libmpg123 should be made dynamic.

You asked what the rationale was - citing disk space as the only
reason. I was saying that disk space is not the only valid reason for
wanting to do this.

But there is a reason for using our own version -- the HeapAlloc/Free
and Wine tracing changes that Aric mentioned in the initial email. So
for that reason, it won't be practical to dynamically link.

At the end of the day, it all boils down to this: what is the simplest
strategy for maintaining the code and updating it in the future.

- Reece

[Juan Lang]

> I was not suggesting that libmpg123 should be made dynamic.
>
> You asked what the rationale was - citing disk space as the only
> reason. I was saying that disk space is not the only valid reason for
> wanting to do this.

I misunderstood you.  Sorry for putting words in your mouth.
--Juan

[Chris Robinson]

On Thursday 13 August 2009 12:03:02 pm Reece Dunn wrote:
> But there is a reason for using our own version -- the HeapAlloc/Free
> and Wine tracing changes that Aric mentioned in the initial email. So
> for that reason, it won't be practical to dynamically link.

Needing to change malloc->HeapAlloc and free->HeapFree is part of a larger 
problem. Other libraries are more important in this regard, but cannot be 
changed (eg. libGL mallocs tons of data, and we can't change that). A better 
way to handle that would be to override malloc (or more specifically mmap) to 
pull from Wine's heap.

There are patches in the works that do this, but there's straggling issues 
that AJ hasn't been able to work out yet. This is something that really needs 
to be done anyway, as GL/D3D are really suffering on cards with a lot of VRAM, 
so I don't think citing that as an issue is valid.

Address space(was: mp3 update)

[Stefan Dösinger]

Am Thursday 13 August 2009 21:19:38 schrieb Chris Robinson:
> There are patches in the works that do this, but there's straggling issues
> that AJ hasn't been able to work out yet. This is something that really
> needs to be done anyway, as GL/D3D are really suffering on cards with a lot
> of VRAM, so I don't think citing that as an issue is valid.

Actually, AJ implemented some memory management improvements, like freeing the 
DOS memory after app startup, so we probably have 1.5 GB address space free 
for OpenGL these days. If any app still crashes due to out of address space 
issues, there's probably a problem going on elsewhere, like an address space 
leak in the driver or Wine.