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
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
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
On Wed, Aug 12, 2009 at 4:34 PM, Chris Robinson<chris.kcat(a)gmail.com> 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 "There is one thing stronger than all the armies in the world, and that is an idea whose time has come." - Victor Hugo
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
On Wed, Aug 12, 2009 at 4:09 PM, Juan Lang<juan.lang(a)gmail.com> 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. -- -Austin
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
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<chris.kcat(a)gmail.com> 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
On Wed, Aug 12, 2009 at 6:30 PM, Aric Stewart<aric(a)codeweavers.com> 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. -- Steven Edwards "There is one thing stronger than all the armies in the world, and that is an idea whose time has come." - Victor Hugo
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.
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
On Thu, Aug 13, 2009 at 1:41 PM, Juan Lang<juan.lang(a)gmail.com> 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... -- -Austin
Austin English <austinenglish(a)gmail.com> writes:
On Thu, Aug 13, 2009 at 1:41 PM, Juan Lang<juan.lang(a)gmail.com> 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. -- Alexandre Julliard julliard(a)winehq.org
2009/8/13 Alexandre Julliard <julliard(a)winehq.org>:
Austin English <austinenglish(a)gmail.com> writes:
On Thu, Aug 13, 2009 at 1:41 PM, Juan Lang<juan.lang(a)gmail.com> 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
On Thu, Aug 13, 2009 at 09:07:29PM +0200, Alexandre Julliard wrote:
Austin English <austinenglish(a)gmail.com> writes:
On Thu, Aug 13, 2009 at 1:41 PM, Juan Lang<juan.lang(a)gmail.com> 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
Marcus Meissner <marcus(a)jet.franken.de> writes:
On Thu, Aug 13, 2009 at 09:07:29PM +0200, Alexandre Julliard wrote:
Austin English <austinenglish(a)gmail.com> writes:
On Thu, Aug 13, 2009 at 1:41 PM, Juan Lang<juan.lang(a)gmail.com> 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. -- Alexandre Julliard julliard(a)winehq.org
Alexandre Julliard wrote:
Marcus Meissner <marcus(a)jet.franken.de> writes:
On Thu, Aug 13, 2009 at 09:07:29PM +0200, Alexandre Julliard wrote:
Austin English <austinenglish(a)gmail.com> writes:
On Thu, Aug 13, 2009 at 1:41 PM, Juan Lang<juan.lang(a)gmail.com> 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
On Fri, Aug 14, 2009 at 12:52:48PM +0200, Michael Stefaniuc wrote:
Alexandre Julliard wrote:
Marcus Meissner <marcus(a)jet.franken.de> writes:
On Thu, Aug 13, 2009 at 09:07:29PM +0200, Alexandre Julliard wrote:
Austin English <austinenglish(a)gmail.com> writes:
On Thu, Aug 13, 2009 at 1:41 PM, Juan Lang<juan.lang(a)gmail.com> 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
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 <marcus(a)jet.franken.de> writes:
On Thu, Aug 13, 2009 at 09:07:29PM +0200, Alexandre Julliard wrote:
Austin English <austinenglish(a)gmail.com> writes:
On Thu, Aug 13, 2009 at 1:41 PM, Juan Lang<juan.lang(a)gmail.com> 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
2009/8/13 Juan Lang <juan.lang(a)gmail.com>:
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
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
2009/8/13 Juan Lang <juan.lang(a)gmail.com>:
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
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
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.
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.
participants (11)
-
Alexandre Julliard -
Aric Stewart -
Austin English -
Chris Robinson -
Juan Lang -
Marcus Meissner -
Marcus Meissner -
Michael Stefaniuc -
Reece Dunn -
Stefan Dösinger -
Steven Edwards