Debian Bug report logs - #715461
libsdl-mixer1.2: no sf2 sound fonts loaded by default

Toggle useless messages

Message #5 received at submit@bugs.debian.org (full text, mbox, reply):

From: Fabian Greffrath <fabian@greffrath.com>

To: Debian Bug Tracking System <submit@bugs.debian.org>

Subject: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Tue, 09 Jul 2013 11:07:04 +0200

Package: libsdl-mixer1.2
Version: 1.2.12-5
Severity: normal

Hello,

the current version of libsdl-mixer1.2 is able to use fluidsynth for midi
playback, which is far superior compared to the embedded copy of timidity which
relies on sound fonts in the obsolete pat format (and thus requires the
freepats package to play music). However, fluidsynth requires sound fonts in
the sf2 format to be able to play music.

To achieve this, two things have to be changed in the current Debian package:
1) Debian currently ships three sf2 sound font files in the fluid-soundfont-gs,
musescore-soundfont-gm and fluid-soundfont-gm packages, respectively. These
packages should get promoted to Recommends with a higher priority than
freepats:
- Recommends: freepats
+ Recommends: musescore-soundfont-gm, fluid-soundfont-gm, freepats
I have given the musescore-soundfont-gm package higher priority, because it is
a complete set but has only 6MB compared to the 145Mb of fluid-soundfont-gm.
fluid-soundfont-gs is out of this list, because I did not get it working
without its bigger brother fluid-soundfont-gm installed alongside.

2) The list of available sound font files is currently set to NULL in
music.c:148 and relies on the SDL_SOUNDFONTS environment variable to contain a
list of appropriate files. However, it should get initialized with the known
locations of the sf2 sound font files available in Debian:
- char* soundfont_paths = NULL;
+ char* soundfont_paths =
"/usr/share/sounds/sf2/TimGM6mb.sf2:/usr/share/sounds/sf2/FluidR3_GM.sf2";

This will enable high quality music playback *by default* (at least higher
quality than using the internal timidity copy with freepats).

 - Fabian



-- System Information:
Debian Release: jessie/sid
  APT prefers stable
  APT policy: (900, 'stable'), (800, 'testing'), (700, 'unstable')
Architecture: amd64 (x86_64)
Foreign Architectures: i386

Kernel: Linux 3.9-1-amd64 (SMP w/4 CPU cores)
Locale: LANG=de_DE.UTF-8, LC_CTYPE=de_DE.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash

Versions of packages libsdl-mixer1.2 depends on:
ii  libc6              2.17-7
ii  libflac8           1.3.0-1
ii  libmad0            0.15.1b-8
ii  libmikmod2         3.1.12-5
ii  libsdl1.2debian    1.2.15-5
ii  libvorbis0a        1.3.2-1.3
ii  libvorbisfile3     1.3.2-1.3
ii  multiarch-support  2.17-7

Versions of packages libsdl-mixer1.2 recommends:
ii  freepats  20060219-1

libsdl-mixer1.2 suggests no packages.

-- no debconf information

Message #10 received at 715461@bugs.debian.org (full text, mbox, reply):

From: Fabian Greffrath <fabian@greffrath.com>

To: 715461@bugs.debian.org

Subject: Re: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Tue, 09 Jul 2013 11:48:15 +0200

Am Dienstag, den 09.07.2013, 11:07 +0200 schrieb Fabian Greffrath: 
> + Recommends: musescore-soundfont-gm, fluid-soundfont-gm, freepats

Recommends: musescore-soundfont-gm | freepats

Of course, these should be alternative dependencies. I have dropped
fluid-soundfont-gm, see below:

> + char* soundfont_paths =
> "/usr/share/sounds/sf2/TimGM6mb.sf2:/usr/share/sounds/sf2/FluidR3_GM.sf2";

char* soundfont_paths = "/usr/share/sounds/sf2/TimGM6mb.sf2";

The Mix_EachSoundFont() function return()s when one of the passed sound
font files cannot be handled. So instead of requiring both sound fonts
to be installed, let's concentrate on one instead (I have decided for
the smaller sound font) and let the user explicitely select the other
one via the SDL_SOUNDFONTS variable.

- Fabian

Message #15 received at 715461@bugs.debian.org (full text, mbox, reply):

From: Fabian Greffrath <fabian@greffrath.com>

To: 715461@bugs.debian.org

Subject: Re: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Tue, 09 Jul 2013 16:08:35 +0200

[Message part 1 (text/plain, inline)]

tags -1 + patch

Am Dienstag, den 09.07.2013, 11:48 +0200 schrieb Fabian Greffrath: 
> The Mix_EachSoundFont() function return()s when one of the passed sound
> font files cannot be handled. So instead of requiring both sound fonts
> to be installed, let's concentrate on one instead (I have decided for
> the smaller sound font) and let the user explicitely select the other
> one via the SDL_SOUNDFONTS variable.

The attached patch/hack fixes this. It iterates over both given sound
font files and uses the last one it finds. That is, if both are present,
the latter overrides the former (in this case, the bigger FluidR3_GM.sf2
overrides the smaller TimGM6mb.sf2). If none is present, it gracefully
falls back to its own internal timidity sound renderer and freepats.

The correct package relationship with this patch is thus:

Recommends: musescore-soundfont-gm, fluid-soundfont-gm, freepats

Hope that helps!

- Fabian

[soundfont_paths.patch (text/x-patch, attachment)]

Message #20 received at 715461@bugs.debian.org (full text, mbox, reply):

From: "Manuel A. Fernandez Montecelo" <manuel.montezelo@gmail.com>

To: Fabian Greffrath <fabian@greffrath.com>, 715461@bugs.debian.org

Subject: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Thu, 11 Jul 2013 12:37:00 +0100

Hi Fabian,

Thanks for the reports and the fixes.

2013/7/9 Fabian Greffrath <fabian@greffrath.com>:
> Recommends: musescore-soundfont-gm, fluid-soundfont-gm, freepats

This again is alternative dependencies, right?

Recommends: musescore-soundfont-gm | fluid-soundfont-gm | freepats


Cheers.
--
Manuel A. Fernandez Montecelo <manuel.montezelo@gmail.com>

Message #25 received at 715461@bugs.debian.org (full text, mbox, reply):

From: Fabian Greffrath <fabian@greffrath.com>

To: "Manuel A. Fernandez Montecelo" <manuel.montezelo@gmail.com>

Cc: 715461@bugs.debian.org

Subject: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Thu, 11 Jul 2013 13:46:31 +0200

Am Donnerstag, den 11.07.2013, 12:37 +0100 schrieb Manuel A. Fernandez
Montecelo: 
> This again is alternative dependencies, right?
> 
> Recommends: musescore-soundfont-gm | fluid-soundfont-gm | freepats

Yes, sure, sorry. I am still not sure, however, if fluid-soundfont-gm
should get precedence over freepats as it is quite chunky and some users
will be happy to hear music at all - regardless of its quality.

Please note that the patch is just a quick hack to demonstrate that
music playback with sdl_mixer using fluidsynth and a packaged sound font
*could* be easy. I am not sure if it is already ready for application in
Debian, maybe could could check that with upstream first? However, I
find the current situation of being forced to set an environment
variable first in order to get music playback of reasonable quality
completely unsatisfying.

- Fabian

Message #30 received at 715461@bugs.debian.org (full text, mbox, reply):

From: "Manuel A. Fernandez Montecelo" <manuel.montezelo@gmail.com>

To: Fabian Greffrath <fabian@greffrath.com>

Cc: 715461@bugs.debian.org

Subject: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Thu, 11 Jul 2013 14:47:50 +0100

2013/7/11 Fabian Greffrath <fabian@greffrath.com>:
> Am Donnerstag, den 11.07.2013, 12:37 +0100 schrieb Manuel A. Fernandez
> Montecelo:
>> This again is alternative dependencies, right?
>>
>> Recommends: musescore-soundfont-gm | fluid-soundfont-gm | freepats
>
> Yes, sure, sorry. I am still not sure, however, if fluid-soundfont-gm
> should get precedence over freepats as it is quite chunky and some users
> will be happy to hear music at all - regardless of its quality.
>
> Please note that the patch is just a quick hack to demonstrate that
> music playback with sdl_mixer using fluidsynth and a packaged sound font
> *could* be easy. I am not sure if it is already ready for application in
> Debian, maybe could could check that with upstream first? However, I
> find the current situation of being forced to set an environment
> variable first in order to get music playback of reasonable quality
> completely unsatisfying.

There are patches doing something similar with freepats for ages, so I
think that it's sensible to try this route with better quality sound.

At some point I would like to disable the internal timidity
completely.  I think that libsdl2-mixer already got rid of the GPL
version of timidity.

I'm going to upload the fixes now, would you please test if it's
working as you expect when you have some time availale?


Cheers.
--
Manuel A. Fernandez Montecelo <manuel.montezelo@gmail.com>

Message #35 received at 715461@bugs.debian.org (full text, mbox, reply):

From: Fabian Greffrath <fabian@greffrath.com>

To: "Manuel A. Fernandez Montecelo" <manuel.montezelo@gmail.com>

Cc: 715461@bugs.debian.org

Subject: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Thu, 11 Jul 2013 15:56:09 +0200

Am Donnerstag, den 11.07.2013, 14:47 +0100 schrieb Manuel A. Fernandez
Montecelo: 
> There are patches doing something similar with freepats for ages, so I
> think that it's sensible to try this route with better quality sound.

Yes, I know that patch. ;)

> I'm going to upload the fixes now, would you please test if it's
> working as you expect when you have some time availale?

Sure thing, please go ahead!

- Fabian

Message #40 received at 715461-close@bugs.debian.org (full text, mbox, reply):

From: mafm@debian.org (Manuel A. Fernandez Montecelo)

To: 715461-close@bugs.debian.org

Subject: Bug#715461: fixed in sdl-mixer1.2 1.2.12-6

Date: Thu, 11 Jul 2013 15:09:20 +0000

Source: sdl-mixer1.2
Source-Version: 1.2.12-6

We believe that the bug you reported is fixed in the latest version of
sdl-mixer1.2, which is due to be installed in the Debian FTP archive.

A summary of the changes between this version and the previous one is
attached.

Thank you for reporting the bug, which will now be closed.  If you
have further comments please address them to 715461@bugs.debian.org,
and the maintainer will reopen the bug report if appropriate.

Debian distribution maintenance software
pp.
Manuel A. Fernandez Montecelo <mafm@debian.org> (supplier of updated sdl-mixer1.2 package)

(This message was generated automatically at their request; if you
believe that there is a problem with it please contact the archive
administrators by mailing ftpmaster@ftp-master.debian.org)


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Format: 1.8
Date: Thu, 11 Jul 2013 12:17:15 +0100
Source: sdl-mixer1.2
Binary: libsdl-mixer1.2 libsdl-mixer1.2-dbg libsdl-mixer1.2-dev
Architecture: source amd64
Version: 1.2.12-6
Distribution: unstable
Urgency: low
Maintainer: Debian SDL packages maintainers <pkg-sdl-maintainers@lists.alioth.debian.org>
Changed-By: Manuel A. Fernandez Montecelo <mafm@debian.org>
Description: 
 libsdl-mixer1.2 - Mixer library for Simple DirectMedia Layer 1.2, libraries
 libsdl-mixer1.2-dbg - Mixer library for Simple DirectMedia Layer 1.2, debugging
 libsdl-mixer1.2-dev - Mixer library for Simple DirectMedia Layer 1.2, development files
Closes: 715447 715461
Changes: 
 sdl-mixer1.2 (1.2.12-6) unstable; urgency=low
 .
   * MIDI enhancements (Thanks Fabian Greffrath for the reports and the
     fixes):
     - Add the "--disable-music-fluidsynth-shared" parameter to ./configure so
       the library is linked to libfluidsynth instead of loaded through dlopen()
       at runtime (Closes: #715447).
     - Add patch bug-715461-soundfont_paths.patch and depend on "sf2" sound fonts
       to have good MIDI sounds by default (Closes: #715461).
Checksums-Sha1: 
 1a39c85494d05b792dc93f26c0f63adfb166c34d 2402 sdl-mixer1.2_1.2.12-6.dsc
 7a913a6a1e03ed3d5544844a06731ae373952f27 14407 sdl-mixer1.2_1.2.12-6.debian.tar.gz
 7ba4c730ce53ee579ca204ae2b1d872a9dc84032 93068 libsdl-mixer1.2_1.2.12-6_amd64.deb
 237fea31c86af7649a161dfa31f4ec6cd03fae92 177768 libsdl-mixer1.2-dbg_1.2.12-6_amd64.deb
 40b9956d56ee7e008458de97b86f66028bbbf2a5 127770 libsdl-mixer1.2-dev_1.2.12-6_amd64.deb
Checksums-Sha256: 
 62b3ef59062516cf312330b3c2faa5281a94e88dbca9a7b1b7c2708e4f811ee2 2402 sdl-mixer1.2_1.2.12-6.dsc
 a861dce23f42161f272264a82a1998811846fa006faaa8fb9dbd0005926f1ac0 14407 sdl-mixer1.2_1.2.12-6.debian.tar.gz
 657af67df54937b47aa6fe34b1cf4ed675149736f392ce47bbc732669636bd89 93068 libsdl-mixer1.2_1.2.12-6_amd64.deb
 1889dcb2f14317429b41f0a9c744f30efd939e99ed2802c33bd64d659b6a329b 177768 libsdl-mixer1.2-dbg_1.2.12-6_amd64.deb
 db354b5ccf9f8211f3fe9b31ce7a5c8e75fbde0dd1b37d6c0c432da55a23186b 127770 libsdl-mixer1.2-dev_1.2.12-6_amd64.deb
Files: 
 5f3a4b428b234539065bafebeed9d242 2402 libs optional sdl-mixer1.2_1.2.12-6.dsc
 28ea5a261a27b7ff150faa4c0d4af5e1 14407 libs optional sdl-mixer1.2_1.2.12-6.debian.tar.gz
 26b3f98d8e44419ce06a4717d218a0bc 93068 libs optional libsdl-mixer1.2_1.2.12-6_amd64.deb
 88f48f52658396a8aca3982335b6bcab 177768 debug extra libsdl-mixer1.2-dbg_1.2.12-6_amd64.deb
 4e69c00f7ec643c4546cc3418ecccacb 127770 libdevel optional libsdl-mixer1.2-dev_1.2.12-6_amd64.deb

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)

iQIcBAEBAgAGBQJR3rZ0AAoJEH92BqRF3KgOQYIQAKk1e03ax3xiHS98kC5jd+4B
/n7f6cNNRebczceI3VH6XpuPmW3LrceozU8geiIkQgJX8Ain3Oa6KMvp5sRTQ/82
qVX3fAvI/hxSsqR1Sx+TvvfDdSccSINwEEjW7nf4BIHBp6Czam57SBX/WjBf7vuF
kO5+5F9wDdaAVX9qt5syzQTXW5H2WWlkDtVu1XvqznsDOBBClXpBYDSgjYyFezDo
j9RAdnMx7dEpLog86BxQmgx7hn0zIdCbP6bLCBniwguDRSxAfd3q8UrINJbSbM8H
OtUsIdVRAkjgp6MsWvQgiIWAMur+io3HC7FQw1yPxFdOLmiq0zeyG0PLC2DzSpLF
+R6soZH07ZQeLHrn9+wXZXciI3MbtqwT0G0MRVjG+QZ8QS7VL5B3yWoWZ0z4Iqr8
YUGfILv0z90QUb0vJseXCUHf4SyOPwm8vK2YjsS3i4LUoBWT312kcb8bcCQZGANA
eQjNmLlvwHAIBs3k9URJKsgwT98luT+IFC04U32tK5CQqXrl3h6SxkGfGKKyv7Vx
BARlHTozuQ/K53baUqfAPGsuSO0TAgbB3LdDcpTlLoxuFvR/reUFqw2R85nAl8DR
Eed3ZelQQpyPkhqxlVjumhadLIMPmuVf0ysgWIFZ/Pw7IO0Dthv4uF1mMV2wT8a4
MLETUtMt0A+Z6eXzJ8O/
=wuG2
-----END PGP SIGNATURE-----

Message #45 received at 715461@bugs.debian.org (full text, mbox, reply):

From: Fabian Greffrath <fabian@greffrath.com>

To: "Manuel A. Fernandez Montecelo" <manuel.montezelo@gmail.com>

Cc: 715461@bugs.debian.org

Subject: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Fri, 12 Jul 2013 11:56:57 +0200

Am Donnerstag, den 11.07.2013, 14:47 +0100 schrieb Manuel A. Fernandez
Montecelo: 
> I'm going to upload the fixes now, would you please test if it's
> working as you expect when you have some time availale?

I'd say it works exactly as expected. Thanks for the prompt upload! :)

- Fabian

Message #50 received at 715461@bugs.debian.org (full text, mbox, reply):

From: "Manuel A. Fernandez Montecelo" <manuel.montezelo@gmail.com>

To: Fabian Greffrath <fabian@greffrath.com>

Cc: 715461@bugs.debian.org

Subject: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Fri, 12 Jul 2013 13:50:28 +0100

2013/7/12 Fabian Greffrath <fabian@greffrath.com>:
> Am Donnerstag, den 11.07.2013, 14:47 +0100 schrieb Manuel A. Fernandez
> Montecelo:
>> I'm going to upload the fixes now, would you please test if it's
>> working as you expect when you have some time availale?
>
> I'd say it works exactly as expected. Thanks for the prompt upload! :)

Thanks to you for the fix!

--
Manuel A. Fernandez Montecelo <manuel.montezelo@gmail.com>

Message #55 received at 715461@bugs.debian.org (full text, mbox, reply):

From: "Manuel A. Fernandez Montecelo" <manuel.montezelo@gmail.com>

To: Fabian Greffrath <fabian@greffrath.com>, 715461@bugs.debian.org, Dominique Dumont <dod@debian.org>, 718129@bugs.debian.org

Subject: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Wed, 7 Aug 2013 22:13:49 +0100

Hi,

For reference, I think that bug-715461-soundfont_paths.patch created
the recent problem in #718129.

Instead of adding another patch (the one fixing #718129, included in
revision -7 of the package), perhaps the pach to include soundfonts
can be modified to avoid having the second one.

I think that, as Dominique mentioned, the problem is that the
SDL_free() frees memory which has not been malloc()ed (SDL_malloc()
perhaps?).

For example, one fix that comes to mind is to change the line in the
first patch:

char* soundfont_paths =
"/usr/share/sounds/sf2/TimGM6mb.sf2:/usr/share/sounds/sf2/FluidR3_GM.sf2";

to this:

char* soundfont_paths =
SDL_strdup("/usr/share/sounds/sf2/TimGM6mb.sf2:/usr/share/sounds/sf2/FluidR3_GM.sf2");

What do you think?  Feels less intrusive than having a second patch.


Cheers.
--
Manuel

Message #60 received at 715461@bugs.debian.org (full text, mbox, reply):

From: Fabian Greffrath <fabian@greffrath.com>

To: "Manuel A. Fernandez Montecelo" <manuel.montezelo@gmail.com>

Cc: 715461@bugs.debian.org, Dominique Dumont <dod@debian.org>, 718129@bugs.debian.org

Subject: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Thu, 08 Aug 2013 06:29:32 +0200

Am Mittwoch, den 07.08.2013, 22:13 +0100 schrieb Manuel A. Fernandez
Montecelo: 
> char* soundfont_paths =
> SDL_strdup("/usr/share/sounds/sf2/TimGM6mb.sf2:/usr/share/sounds/sf2/FluidR3_GM.sf2");
> 
> What do you think?  Feels less intrusive than having a second patch.

Good idea! Feels much cleaner than having a second patch remove
unrelated code.

- Fabian

Message #65 received at 715461@bugs.debian.org (full text, mbox, reply):

From: Dominique Dumont <dod@debian.org>

To: "Manuel A. Fernandez Montecelo" <manuel.montezelo@gmail.com>, Fabian Greffrath <fabian@greffrath.com>, 715461@bugs.debian.org, 718129@bugs.debian.org

Subject: Re: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Thu, 08 Aug 2013 18:44:38 +0200

[Message part 1 (text/plain, inline)]

On Wednesday 07 August 2013 22:13:49 you wrote:
> For example, one fix that comes to mind is to change the line in the
> first patch:
> 
> char* soundfont_paths =
> "/usr/share/sounds/sf2/TimGM6mb.sf2:/usr/share/sounds/sf2/FluidR3_GM.sf2";
> 
> to this:
> 
> char* soundfont_paths =
> SDL_strdup("/usr/share/sounds/sf2/TimGM6mb.sf2:/usr/share/sounds/sf2/FluidR3
> _GM.sf2");
> 
> What do you think?  Feels less intrusive than having a second patch.

ok to reduce the number of patches.

But the SDL_strdup solution is needlessly complicated and will probably have 
some eyebrows raised very high in the future. 

I'd rather see bug-718129-rm-bad-free.patch merged into bug-715461-
soundfont_paths.patch so as to have one simple, correct patch.

All the best

[signature.asc (application/pgp-signature, inline)]

Message #70 received at 715461@bugs.debian.org (full text, mbox, reply):

From: "Manuel A. Fernandez Montecelo" <manuel.montezelo@gmail.com>

To: Dominique Dumont <dod@debian.org>

Cc: Fabian Greffrath <fabian@greffrath.com>, 715461@bugs.debian.org, 718129@bugs.debian.org

Subject: Re: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Thu, 8 Aug 2013 19:13:24 +0100

2013/8/8 Dominique Dumont <dod@debian.org>:
> On Wednesday 07 August 2013 22:13:49 you wrote:
>> For example, one fix that comes to mind is to change the line in the
>> first patch:
>>
>> char* soundfont_paths =
>> "/usr/share/sounds/sf2/TimGM6mb.sf2:/usr/share/sounds/sf2/FluidR3_GM.sf2";
>>
>> to this:
>>
>> char* soundfont_paths =
>> SDL_strdup("/usr/share/sounds/sf2/TimGM6mb.sf2:/usr/share/sounds/sf2/FluidR3
>> _GM.sf2");
>>
>> What do you think?  Feels less intrusive than having a second patch.
>
> ok to reduce the number of patches.
>
> But the SDL_strdup solution is needlessly complicated and will probably have
> some eyebrows raised very high in the future.
>
> I'd rather see bug-718129-rm-bad-free.patch merged into bug-715461-
> soundfont_paths.patch so as to have one simple, correct patch.

I don't know if my intentions were clear.

I meant to modify the first patch bug-715461-soundfont_paths.patch so
when that variable "soundfont_paths" is assigned, it's done with
SDL_strdup() (it's done in several places in the code --that's where I
got the idea from--, so "it fits"), and remove the second patch
altogether, bug-718129-rm-bad-free.patch.

I think that this fits the "simple, correct patch" idea that you
mention, and I don't see anything complicated about it -- it's just to
assign the variable with dynamic memory, which is the way the rest of
the code thinks that it should be (there are more instances trying to
free memory from this varaible).

The variable can be set by users of the library to use dynamic memory
[1], so removing that SDL_free() is theoretically incorrect -- if it
gets assigned other content in runtime, it would not free it where the
SDL_free() is removed (which is the end of the program, so actually it
shoudn't be that important, bug e.g. valgrind would report it as a
leak).

Still, if anybody thinks that other solutions are preferrable, it's OK
by me -- I have no special interest in pushing this solution over
others.  I volunteer to fix this, no matter the solution chosen, if
nobody else wants.

And Dominique, sorry that I didn't catch this when you asked me, I was
busy at work and couldn't pay full attention to the issue.


Cheers.

[1] music.c, int Mix_SetSoundFonts(const char *paths)

-- 
Manuel A. Fernandez Montecelo <manuel.montezelo@gmail.com>

Message #75 received at 715461@bugs.debian.org (full text, mbox, reply):

From: Fabian Greffrath <fabian@greffrath.com>

To: manuel.montezelo@gmail.com, dod@debian.org

Cc: fabian@greffrath.com, 715461@bugs.debian.org, 718129@bugs.debian.org

Subject: AW: Re: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Thu, 08 Aug 2013 21:53:44 +0200

[Message part 1 (text/plain, inline)]

As the one who introduced that bug: please go ahead, your solution looks right to me. ;-)

Von Samsung Mobile gesendet

-------- Ursprüngliche Nachricht --------
Von: "Manuel A. Fernandez Montecelo" <manuel.montezelo@gmail.com> 
Datum:  
An: Dominique Dumont <dod@debian.org> 
Cc: Fabian Greffrath <fabian@greffrath.com>,715461@bugs.debian.org,718129@bugs.debian.org 
Betreff: Re: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default 
 
2013/8/8 Dominique Dumont <dod@debian.org>:
> On Wednesday 07 August 2013 22:13:49 you wrote:
>> For example, one fix that comes to mind is to change the line in the
>> first patch:
>>
>> char* soundfont_paths =
>> "/usr/share/sounds/sf2/TimGM6mb.sf2:/usr/share/sounds/sf2/FluidR3_GM.sf2";
>>
>> to this:
>>
>> char* soundfont_paths =
>> SDL_strdup("/usr/share/sounds/sf2/TimGM6mb.sf2:/usr/share/sounds/sf2/FluidR3
>> _GM.sf2");
>>
>> What do you think?  Feels less intrusive than having a second patch.
>
> ok to reduce the number of patches.
>
> But the SDL_strdup solution is needlessly complicated and will probably have
> some eyebrows raised very high in the future.
>
> I'd rather see bug-718129-rm-bad-free.patch merged into bug-715461-
> soundfont_paths.patch so as to have one simple, correct patch.

I don't know if my intentions were clear.

I meant to modify the first patch bug-715461-soundfont_paths.patch so
when that variable "soundfont_paths" is assigned, it's done with
SDL_strdup() (it's done in several places in the code --that's where I
got the idea from--, so "it fits"), and remove the second patch
altogether, bug-718129-rm-bad-free.patch.

I think that this fits the "simple, correct patch" idea that you
mention, and I don't see anything complicated about it -- it's just to
assign the variable with dynamic memory, which is the way the rest of
the code thinks that it should be (there are more instances trying to
free memory from this varaible).

The variable can be set by users of the library to use dynamic memory
[1], so removing that SDL_free() is theoretically incorrect -- if it
gets assigned other content in runtime, it would not free it where the
SDL_free() is removed (which is the end of the program, so actually it
shoudn't be that important, bug e.g. valgrind would report it as a
leak).

Still, if anybody thinks that other solutions are preferrable, it's OK
by me -- I have no special interest in pushing this solution over
others.  I volunteer to fix this, no matter the solution chosen, if
nobody else wants.

And Dominique, sorry that I didn't catch this when you asked me, I was
busy at work and couldn't pay full attention to the issue.


Cheers.

[1] music.c, int Mix_SetSoundFonts(const char *paths)

-- 
Manuel A. Fernandez Montecelo <manuel.montezelo@gmail.com>

[Message part 2 (text/html, inline)]

Message #80 received at 715461@bugs.debian.org (full text, mbox, reply):

From: Dominique Dumont <dod@debian.org>

To: "Manuel A. Fernandez Montecelo" <manuel.montezelo@gmail.com>

Cc: Fabian Greffrath <fabian@greffrath.com>, 715461@bugs.debian.org, 718129@bugs.debian.org

Subject: Re: Re: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Fri, 09 Aug 2013 09:51:24 +0200

[Message part 1 (text/plain, inline)]

On Thursday 08 August 2013 19:13:24 Manuel A. Fernandez Montecelo wrote:
> I meant to modify the first patch bug-715461-soundfont_paths.patch so
> when that variable "soundfont_paths" is assigned, it's done with
> SDL_strdup() (it's done in several places in the code --that's where I
> got the idea from--, so "it fits"), and remove the second patch
> altogether, bug-718129-rm-bad-free.patch.

Understood.

> The variable can be set by users of the library to use dynamic memory
> [1], so removing that SDL_free() is theoretically incorrect -- if it
> gets assigned other content in runtime, it would not free it where the
> SDL_free() is removed (which is the end of the program, so actually it
> shoudn't be that important, bug e.g. valgrind would report it as a
> leak).

I think "soundfont_paths" initialisation should be done in Mix_Init().

Otherwise a sequence of Mix_Init, Mix_Quit, Mix_Init and Mix_Quit will lead to 
a segfault. This sequence may not make sense from a user's point of view, but 
it may happen in test suites like SDL-perl's test suite.

> And Dominique, sorry that I didn't catch this when you asked me, I was
> busy at work and couldn't pay full attention to the issue.

Don't worry about it. Been there, done that ;-)

All the best

[signature.asc (application/pgp-signature, inline)]

Message #85 received at 715461@bugs.debian.org (full text, mbox, reply):

From: "Manuel A. Fernandez Montecelo" <manuel.montezelo@gmail.com>

To: Dominique Dumont <dod@debian.org>, 718129@bugs.debian.org

Cc: Fabian Greffrath <fabian@greffrath.com>, 715461@bugs.debian.org

Subject: Re: Bug#718129: Re: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Mon, 12 Aug 2013 00:29:34 +0100

2013/8/9 Dominique Dumont <dod@debian.org>:
> On Thursday 08 August 2013 19:13:24 Manuel A. Fernandez Montecelo wrote:
>> I meant to modify the first patch bug-715461-soundfont_paths.patch so
>> when that variable "soundfont_paths" is assigned, it's done with
>> SDL_strdup() (it's done in several places in the code --that's where I
>> got the idea from--, so "it fits"), and remove the second patch
>> altogether, bug-718129-rm-bad-free.patch.
>
> Understood.
>
>> The variable can be set by users of the library to use dynamic memory
>> [1], so removing that SDL_free() is theoretically incorrect -- if it
>> gets assigned other content in runtime, it would not free it where the
>> SDL_free() is removed (which is the end of the program, so actually it
>> shoudn't be that important, bug e.g. valgrind would report it as a
>> leak).
>
> I think "soundfont_paths" initialisation should be done in Mix_Init().
>
> Otherwise a sequence of Mix_Init, Mix_Quit, Mix_Init and Mix_Quit will lead to
> a segfault. This sequence may not make sense from a user's point of view, but
> it may happen in test suites like SDL-perl's test suite.
>
>> And Dominique, sorry that I didn't catch this when you asked me, I was
>> busy at work and couldn't pay full attention to the issue.
>
> Don't worry about it. Been there, done that ;-)
>
> All the best

So... does this look OK to both of you (I didn't actually upload,
waiting for your confirmation)?

The commit diff is needlessly long due to issues with dos/unix line
ending, but I guess that you get the idea comparing previous and
current versions of the patch.

commit diff:
http://anonscm.debian.org/gitweb/?p=pkg-sdl/packages/sdl-mixer1.2.git;a=commitdiff;h=ef1b4991e313ea2ef840af3291e6e8b77f9e60be

current version of the patch:
http://anonscm.debian.org/gitweb/?p=pkg-sdl/packages/sdl-mixer1.2.git;a=blob;f=debian/patches/bug-715461-soundfont_paths.patch;h=bf2fe632a3ea38de28a079138e7ab3627d7fdd57;hb=ef1b4991e313ea2ef840af3291e6e8b77f9e60be

previous version:
http://anonscm.debian.org/gitweb/?p=pkg-sdl/packages/sdl-mixer1.2.git;a=blob;f=debian/patches/bug-715461-soundfont_paths.patch;h=71faacc115df02713bae2e6211368bc24d2577cc;hb=06b5b0ea8a1417ca356e5055ce5855df8dbfcbd6


Cheers.
-- 
Manuel A. Fernandez Montecelo <manuel.montezelo@gmail.com>

Message #90 received at 715461@bugs.debian.org (full text, mbox, reply):

From: Dominique Dumont <dod@debian.org>

To: "Manuel A. Fernandez Montecelo" <manuel.montezelo@gmail.com>

Cc: 718129@bugs.debian.org, Fabian Greffrath <fabian@greffrath.com>, 715461@bugs.debian.org

Subject: Re: Re: Bug#718129: Re: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Mon, 12 Aug 2013 21:51:18 +0200

[Message part 1 (text/plain, inline)]

On Monday 12 August 2013 00:29:34 Manuel A. Fernandez Montecelo wrote:
> So... does this look OK to both of you (I didn't actually upload,
> waiting for your confirmation)?

Looks good. SDL-perl tests are fine with your updated patch

All the best

[signature.asc (application/pgp-signature, inline)]

Message #95 received at 715461@bugs.debian.org (full text, mbox, reply):

From: "Manuel A. Fernandez Montecelo" <manuel.montezelo@gmail.com>

To: Dominique Dumont <dod@debian.org>, 715461@bugs.debian.org

Cc: 718129@bugs.debian.org, Fabian Greffrath <fabian@greffrath.com>

Subject: Re: Bug#715461: Re: Bug#718129: Re: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Mon, 12 Aug 2013 20:58:05 +0100

2013/8/12 Dominique Dumont <dod@debian.org>:
> On Monday 12 August 2013 00:29:34 Manuel A. Fernandez Montecelo wrote:
>> So... does this look OK to both of you (I didn't actually upload,
>> waiting for your confirmation)?
>
> Looks good. SDL-perl tests are fine with your updated patch
>
> All the best

Updating then, thanks!

-- 
Manuel A. Fernandez Montecelo <manuel.montezelo@gmail.com>

Message #100 received at 715461@bugs.debian.org (full text, mbox, reply):

From: Fabian Greffrath <fabian@greffrath.com>

To: "Manuel A. Fernandez Montecelo" <manuel.montezelo@gmail.com>

Cc: Dominique Dumont <dod@debian.org>, 718129@bugs.debian.org, 715461@bugs.debian.org

Subject: Re: Bug#718129: Re: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Tue, 20 Aug 2013 09:58:40 +0200

Sorry for my late response, I have been on vacation last week.

Am Montag, den 12.08.2013, 00:29 +0100 schrieb Manuel A. Fernandez
Montecelo: 
> So... does this look OK to both of you (I didn't actually upload,
> waiting for your confirmation)?

Yes, it does. However, I would have added a check if the pointer is
already set prior to resetting it, e.g. 

if (!soundfont_paths)
soundfont_paths = SDL_strdup(...);

But this is really just nit-picking.

Thanks for the fix!

- Fabian

Message #105 received at 715461@bugs.debian.org (full text, mbox, reply):

From: Fabian Greffrath <fabian@greffrath.com>

To: "Manuel A. Fernandez Montecelo" <manuel.montezelo@gmail.com>

Cc: Dominique Dumont <dod@debian.org>, 718129@bugs.debian.org, 715461@bugs.debian.org

Subject: Re: Bug#718129: Re: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Mon, 26 Aug 2013 12:25:52 +0200

Am Dienstag, den 20.08.2013, 09:58 +0200 schrieb Fabian Greffrath: 
> Yes, it does. However, I would have added a check if the pointer is
> already set prior to resetting it, e.g. 
> 
> if (!soundfont_paths)
> soundfont_paths = SDL_strdup(...);
> 
> But this is really just nit-picking.

Wait, does SDL_strdup() allocate new memory and isn't the current
solution leaking memory when Mix_Init() is repeatedly called?

- Fabian

Message #110 received at 715461@bugs.debian.org (full text, mbox, reply):

From: "Manuel A. Fernandez Montecelo" <manuel.montezelo@gmail.com>

To: Fabian Greffrath <fabian@greffrath.com>

Cc: Dominique Dumont <dod@debian.org>, 718129@bugs.debian.org, 715461@bugs.debian.org

Subject: Re: Bug#718129: Re: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Fri, 30 Aug 2013 11:50:46 +0100

2013/8/26 Fabian Greffrath <fabian@greffrath.com>:
> Am Dienstag, den 20.08.2013, 09:58 +0200 schrieb Fabian Greffrath:
>> Yes, it does. However, I would have added a check if the pointer is
>> already set prior to resetting it, e.g.
>>
>> if (!soundfont_paths)
>> soundfont_paths = SDL_strdup(...);
>>
>> But this is really just nit-picking.
>
> Wait, does SDL_strdup() allocate new memory and isn't the current
> solution leaking memory when Mix_Init() is repeatedly called?

Yes, I don't think that it will be a real issue except if programs do
real weird things (even the tests are not likely to exhaust any memory
doing this).  But it can be fixed properly anyway, just didn't find
the time to do that.

What it concerns me more is to have to carry the patch around,
modifying it for SDL2, etc.

Do you think that it could be modified to be less system dependent, in
a way that it would be accepted upstream?  Perhaps passing it at
compilation time, instead of having to patch or export at runtime?
Would they be interested in something like this at all?


Cheers.
-- 
Manuel A. Fernandez Montecelo <manuel.montezelo@gmail.com>

Message #115 received at 715461@bugs.debian.org (full text, mbox, reply):

From: Fabian Greffrath <fabian@greffrath.com>

To: "Manuel A. Fernandez Montecelo" <manuel.montezelo@gmail.com>

Cc: Dominique Dumont <dod@debian.org>, 718129@bugs.debian.org, 715461@bugs.debian.org

Subject: Re: Bug#718129: Re: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Sun, 01 Sep 2013 16:37:55 +0200

[Message part 1 (text/plain, inline)]

Am Freitag, den 30.08.2013, 11:50 +0100 schrieb Manuel A. Fernandez
Montecelo: 
> Do you think that it could be modified to be less system dependent, in
> a way that it would be accepted upstream?  Perhaps passing it at
> compilation time, instead of having to patch or export at runtime?

How about the attached modified patch and also the one for debian/rules?

- Fabian

[bug-715461-soundfont_paths.patch (text/x-patch, attachment)]

[sdl-mixer1.2_debian-rules.patch (text/x-patch, attachment)]

Message #120 received at 715461@bugs.debian.org (full text, mbox, reply):

From: Fabian Greffrath <fabian@greffrath.com>

To: "Manuel A. Fernandez Montecelo" <manuel.montezelo@gmail.com>

Cc: Dominique Dumont <dod@debian.org>, 718129@bugs.debian.org, 715461@bugs.debian.org

Subject: Re: Bug#718129: Re: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Sun, 01 Sep 2013 16:44:37 +0200

In the patch for debian/rules the quotation marks have to get escaped.
This somehow got lost by attaching it to my mail.

- Fabian

Message #125 received at 715461@bugs.debian.org (full text, mbox, reply):

From: "Manuel A. Fernandez Montecelo" <manuel.montezelo@gmail.com>

To: Fabian Greffrath <fabian@greffrath.com>, 715461@bugs.debian.org

Cc: Dominique Dumont <dod@debian.org>, 718129@bugs.debian.org

Subject: Re: Bug#715461: Bug#718129: Re: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Fri, 13 Sep 2013 22:31:50 +0100

2013/9/1 Fabian Greffrath <fabian@greffrath.com>:
> In the patch for debian/rules the quotation marks have to get escaped.
> This somehow got lost by attaching it to my mail.

Sorry for the delay but I'm quite busy at the moment.  I will try to
review and get this new patch integrated soonish.  Thanks.


Cheers.
-- 
Manuel A. Fernandez Montecelo <manuel.montezelo@gmail.com>

Message #130 received at 715461@bugs.debian.org (full text, mbox, reply):

From: "Manuel A. Fernandez Montecelo" <manuel.montezelo@gmail.com>

To: Fabian Greffrath <fabian@greffrath.com>, 715461@bugs.debian.org

Subject: Re: Bug#715461: Bug#718129: Re: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

Date: Wed, 9 Oct 2013 19:47:53 +0100

Hi,

I just uploaded another fix for this in -9, thanks Fabian.

http://anonscm.debian.org/gitweb/?p=pkg-sdl/packages/sdl-mixer1.2.git;a=commitdiff;h=396542188f997d14a066d8ddcb4007ca72667509

If you could check it when you have the opportunity and report here
that everything works as expected, it would be great.

Cheers.
--
Manuel

Send a report that this bug log contains spam.