Bug 430417 - Review Request: PersonalCopy-Lite-soundfont - Lite version of the PersonalCopy General Midi soundfont
Review Request: PersonalCopy-Lite-soundfont - Lite version of the PersonalCop...
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On: 430418
Blocks: 226492
  Show dependency treegraph
 
Reported: 2008-01-27 17:02 EST by Hans de Goede
Modified: 2008-03-04 14:48 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-03-04 14:48:09 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Hans de Goede 2008-01-27 17:02:04 EST
Spec URL: http://people.atrpms.net/~hdegoede/PersonalCopy-Lite-soundfont.spec
SRPM URL: http://people.atrpms.net/~hdegoede/PersonalCopy-Lite-soundfont-4.1-1.fc9.src.rpm
Description:
Lite (smaller) version of the PersonalCopy General Midi soundfont in soundfont
2.0 (.sf2) format.
Comment 1 Hans de Goede 2008-01-27 17:03:14 EST
Notice, this requires soundfont-utils (part of gt) to build, gt's review is bug
430418.
Comment 2 Mamoru TASAKA 2008-01-30 13:27:44 EST
Once gt hits rawhide, I will start to check this.
Comment 3 Mamoru TASAKA 2008-02-01 06:06:06 EST
Rebuild failed...
http://koji.fedoraproject.org/koji/taskinfo?taskID=388735

Perhaps some typo?
Comment 4 Hans de Goede 2008-02-01 07:13:07 EST
Nope it got build on a ppc and unsf (part of soundfont-utils / gt) says:
"Error: bad SoundFont header"

So there seem to be some endian issues in unsf, I'm looking into this.
Comment 5 Mamoru TASAKA 2008-02-01 10:50:32 EST
Now for rebuilding this it seems happy with new gt, however
would you check the build.log?

http://koji.fedoraproject.org/koji/taskinfo?taskID=389084
http://koji.fedoraproject.org/koji/getfile?taskID=389085&name=build.log

Are the messages like:
Strange... no valid layers found in instrument GrandPiano bank 0 prog 0
Could not create patch GrandPiano for bank PCLite-B0
	layer 1 of 1 layer(s)
as you expect?
Comment 6 Hans de Goede 2008-02-01 11:31:33 EST
Thanks, I also did a scratch build ymself, but that ended up on an intel
builder, then I tried again, but the upload to koji is slow from here.

I <think> I know whats causing this (the messages aren't normal). PPC has char
default to unsigned <sigh>. A believed fixed gt build is on its way.
Comment 7 Mamoru TASAKA 2008-02-02 06:45:42 EST
Well, I saw the license text and for me the term 3
----------------------------------------------------------------------------
3) Neither the PersonalCopy soundfonts nor any of their individual
    components, in Original or Modified Versions, may be sold by itself.
----------------------------------------------------------------------------
is problematic.
Comment 8 Hans de Goede 2008-02-02 10:33:49 EST
Unblocking FE-Legal, I've already send the license to Spot and it has been
ok-ed. Also notice that the same text is in the SIL font license and we also
ship fonts under this license.
Comment 9 Mamoru TASAKA 2008-02-02 10:41:05 EST
(In reply to comment #8)
> Unblocking FE-Legal, I've already send the license to Spot and it has been
> ok-ed. Also notice that the same text is in the SIL font license and we also
> ship fonts under this license.
> 

Then would you add the mail to rpm?
Comment 10 Hans de Goede 2008-02-02 10:49:49 EST
(In reply to comment #9)
> (In reply to comment #8)
> > Unblocking FE-Legal, I've already send the license to Spot and it has been
> > ok-ed. Also notice that the same text is in the SIL font license and we also
> > ship fonts under this license.
> > 
> 
> Then would you add the mail to rpm?

You mean Spot's mail, why? We normally never put license approval mails in
rpm's, I don't see how this is any different.

As said the same clause in the SIL font license see:
http://scripts.sil.org/cms/scripts/page.php?site_id=nrsi&id=OFL_web

Its clause 1, then see:
http://fedoraproject.org/wiki/Licensing#head-39539a5e2fb8ee8b89a4753ef071f606dd55161f

And notice how the SIL open font license is not only an approved but even is the
recommended font license for Fedora.
Comment 11 Mamoru TASAKA 2008-02-02 11:47:45 EST
Well, okay. I will recheck this later.

A question:
Do you have any idea to deal with %_sysconfdir/timidity.cfg?
What I am thinking now is
- Each rpms providing timidity++ has its own patch config file
  under %_sysconfdir/timidity-patches/, with the name having some
  prefix indicating priority, for example
  %_sysconfdir/timidity-patches/00-soundfont.cfg

- And when a rpm providing timidity patches is installed, it calls
  a script in timidity++-base (for example) to update
  %_sysconfdir/timidity.cfg (marked as %verify(not size md5 mtime) )

- Sysadmin can "edit" timidity.cfg by adding his/her own configuration
  file under %_sysconfdir/timidity-patches/ and calling registration
  script.
Comment 12 Hans de Goede 2008-02-03 14:53:52 EST
Adding Jindrich to the CC because we co-maintain timidity++

(In reply to comment #11)
> Well, okay. I will recheck this later.
> 
> A question:
> Do you have any idea to deal with %_sysconfdir/timidity.cfg?
> What I am thinking now is
> - Each rpms providing timidity++ has its own patch config file
>   under %_sysconfdir/timidity-patches/, with the name having some
>   prefix indicating priority, for example
>   %_sysconfdir/timidity-patches/00-soundfont.cfg
> 
> - And when a rpm providing timidity patches is installed, it calls
>   a script in timidity++-base (for example) to update
>   %_sysconfdir/timidity.cfg (marked as %verify(not size md5 mtime) )
> 
> - Sysadmin can "edit" timidity.cfg by adding his/her own configuration
>   file under %_sysconfdir/timidity-patches/ and calling registration
>   script.
> 

I like the idea of having a directory with per soundfont timidity.cfg for the
patch config files. But I'm not sure this is the best way forward ...

First of all its important to understand that there are 2 users of
/etc/timidity.cfg :
1) timidity(++) itself a full midi synth meant for musicians mostly 
2) timidity deratives, consisting of atleast: SDL_mixer, allegro, libtimidity,
   wildmidi. Which are all midi playing libs mean't for embedding in another
   application and typically have less sound quality and matching much less CPU
   usage.

One of the problems is the timidity deratives only support a subset of
/etc/timidity.cfg syntax. Most noteworthy timidity++ supports a new soundfont
statement, which allows it to directly use .sf2 soundfonts / patches, which
means the whole conversion to GUS patch format can be skipped, and importantly,
since .sf2 is a much newer format it also includes some additional details
making timidity++ sound significantly better when directly using the .sf2 file.

Also the priority ranking you propose is impossible IMHO. With soundfont's there
always is a tradeoff between size and quality. Which not only involves diskspace
usage, but also involves memory usage when playing back a midi file.

Clearly the best tradeoff between memory usage and quality lies differently
between the standalone timidity++ and the versions meant for embedding in other
programs and the tradeoff also is dependent on the hardware capabilities of the
used system.

Well that about concludes describing the problem.

---

Now to a solution. The solution I would like to propose is to make the 2
different usage groups mentioned above (stand alone player versus embedded  into
other applications) use 2 different config files.

I propose to use /etc/timidity.cfg for the embedded case, as that is what all 4
embedded variants currently default too. Also by having an /etc/timidity.cfg
which does not use any of the later timidity.cfg extensions added by timidity++
there will always be an /etc/timidity.cfg present which is compatible with
whatever is looking for it, even manually installed applications.

timidity++ will then be patches to first search for /etc/timidity++.cfg and only
if that is not present fallback to /etc/timidity.cfg. So that a different cfg
file can be made and used for the standalone player case.

The idea is to have /etc/timidity++.cfg only contain a single "soundfont ....
.sf2" line, as the standalone player performs best with a .sf2 file.

The default versions of both .cfg files will get some comment lines at the top
explaining the difference, for /etc/timidity.cfg this will be:
# Warning do not modify this file if you want to change the setting for
# timidity++, the standalone midi player. This file contains a timidity
# compatible patch configuration for applications / libraries which want GUS /
# timidty format patches, like for example SDL_mixer, allegro and libtimidity.
#
# If you want to change the configuration for timidity++, edit /etc/timidity++.cfg
# instead. Only change this file if you want to change the configuration for
# other GUS / timidity format patch using applications

---

With this mechanism (2 config files) in place I don't think there is a need to
have a dir with patch config files. For /etc/timidity.cfg one best breed
compromise between size and quality can be choosen (keeping in mind the embedded
nature of its users). Given that I've currently only found 2 soundfonts which
can be freely redistributed, this and plain PersonalCopy, the choose is easy.
This one, as plain PersonalCopy is twice the size and thats way too big for the
embedded usage scenario.

As for /etc/timidity++.cfg, determining which soundfont is the best is going to
be hard, if not impossible. But there is no need for any per soundfont config
file storage, as each .sf2 file is a standalone unit including all needed cfg
settings in the file itself, all thats needed in /etc/timidity++.cfg is just a
single line pointing to the soundfont one wants to use.

---

This leaves the question howto make sure atleast a single soundfont gets
installed when timidity++ gets installed, for this I would like to suggest
making timidity++ having a Requires on the virtual provides soundfont2, and make
all soundfont packages Provide: soundfont2. That and all .sf2 files should be
installed under /usr/share/soundfonts.

Then /etc/timidity++.cfg can be generated in a %post (install only not upgrade)
to point to the first .sf2 file listed in usr/share/soundfonts . There will
always be atleast one such file because of the Requires: soundfont2
Comment 13 Mamoru TASAKA 2008-02-05 08:17:49 EST
Well, I want to know how Jindrich thinks about your idea.

(In reply to comment #12)
<all snip>
Your idea seems good to me.
Comment 14 Mamoru TASAKA 2008-02-17 07:18:01 EST
Jindrich, would you check the comment 12 by Hans?
Comment 15 Jindrich Novy 2008-02-18 07:33:18 EST
Sure, generally I like design proposed by Hans in comment #12. I like the idea
of two user groups design, one designed for embedding into other apps and
another patchset for music composers. Mamoru's approach seems to be more
general, I don't like timidity++.cfg creation in %post, where the configuration
would be dependent on random patchset state in /usr/share/soundfonts/. The new
soundfonts really should have virtual provides such as soundfont2 as proposed by
Hans.
Comment 16 Hans de Goede 2008-02-19 05:46:15 EST
(In reply to comment #15)
> Sure, generally I like design proposed by Hans in comment #12. I like the idea
> of two user groups design, one designed for embedding into other apps and
> another patchset for music composers.

Good.

> I don't like timidity++.cfg creation in %post, where the configuration
> would be dependent on random patchset state in /usr/share/soundfonts/. The new
> soundfonts really should have virtual provides such as soundfont2 as proposed by
> Hans.

OKay,

I agree that generating timidity++.cfg on the fly isn't pretty. An alternative
would be to add virtual provides: soundfont2 to all soundfont packages. But make
timidity++ explicitly Requires PersonalCopy-Lite-soundfont, and point to that in
timidity++.cfg.

Advantages:
-no ugly kludges to generate /etc/timidity++.cfg
-a reasonable default, if we trust on virtual provides only, then the default
 will become the shortest package
Downsides:
-people who want to use another soundfont, must still have
 PersonalCopy-Lite-soundfont installed because of the deps, taking up diskspace

I think that given no a days large disks, the downside is survivable and this is
a reasonable compromise. Agreed?
Comment 17 Mamoru TASAKA 2008-02-27 09:15:28 EST
I prefer to have require PersonalCopy-Lite-soundfont timidity++
explicitly rather than to use virtual provides.
Comment 18 Hans de Goede 2008-02-27 15:05:47 EST
(In reply to comment #17)
> I prefer to have require PersonalCopy-Lite-soundfont timidity++
> explicitly rather than to use virtual provides.

I assume you mean "I prefer to have timidity++ require
PersonalCopy-Lite-soundfont", right? That is what I was suggesting too,
appearantly I wasn't clear.

Please let me know if you agree that the solution for timidty++ is to require
PersonalCopy-Lite-soundfont, and have an /etc/timidity++.cfg pointing to this
soundfont. Then I can do a new version of this package and we can hopefully
finish this review soon.
Comment 19 Mamoru TASAKA 2008-02-27 21:58:17 EST
(In reply to comment #18)
> (In reply to comment #17)
> > I prefer to have require PersonalCopy-Lite-soundfont timidity++
> > explicitly rather than to use virtual provides.
> 
> I assume you mean "I prefer to have timidity++ require
> PersonalCopy-Lite-soundfont", right? 

Yes... sorry :)
Comment 20 Hans de Goede 2008-02-28 11:20:06 EST
New version:

* Thu Feb 28 2008 Hans de Goede <j.w.r.degoede@hhs.nl> 4.1-2
- Stop providing timidity++-soundfont and /etc/timidity++.cfg, instead
  timidity++ itself will come with an /etc/timidity++.cfg and Require its
  default soundfont (bz 430417)

Go grab it here:
Spec URL: http://people.atrpms.net/~hdegoede/PersonalCopy-Lite-soundfont.spec
SRPM URL:
http://people.atrpms.net/~hdegoede/PersonalCopy-Lite-soundfont-4.1-2.fc9.src.rpm


I've also prepared a matching timidity++ release:
* Thu Feb 28 2008 Hans de Goede <j.w.r.degoede@hhs.nl> 2.13.2-12
- Stop shipping a timidity++-patches package, investigation into the license
  of the included patches has turned up doubts about the rights of the author
  of the midas SGI midi player to release these into the Public Domain
- Instead require PersonalCopy-Lite-soundfont, and point to PCLite.sf2 in
  timidity++.cfg
- Note PersonalCopy-Lite-soundfont also has a PersonalCopy-Lite-patches
  sub-package with the .sf2 file converted to GUS patch format for other
  applications who require timidity++-patches to get GUS format patches, this
  package contains an /etc/timidity.cfg file pointing to the gus patches,
  therefor the timidity++ package now ships a timidty++.cfg instead of a
  timidity.cfg
- Check for /etc/timidity++.cfg before trying /etc/timidity.cfg, see above for
  rationale

Grab it here:
http://people.atrpms.net/~hdegoede/timidity++.spec
http://people.atrpms.net/~hdegoede/timidity++-2.13.2-13.fc9.src.rpm
Comment 21 Mamoru TASAKA 2008-03-03 10:56:16 EST
Looks good, and now I am now listening to MIDI file with timidity++.
Then:

- Fix version <-> %changelog entry mismatch.
- By the way where this version "4.2" came?  (Note:
  I could not find 4.1-2.fc9 srpm, found 4.2-2.fc9 srpm)
- And (for now) the URL seems 404?
Comment 22 Hans de Goede 2008-03-03 14:15:54 EST
(In reply to comment #21)
> Looks good, and now I am now listening to MIDI file with timidity++.
> Then:
> 
> - Fix version <-> %changelog entry mismatch.
Oops, sorry, changelog was correct, for some reason I bumped both the version
and release with the last iteration. Version changed back to 4.1

> - By the way where this version "4.2" came?  (Note:
>   I could not find 4.1-2.fc9 srpm, found 4.2-2.fc9 srpm)
> - And (for now) the URL seems 404?
The URL:
http://www.personalcopy.com/sfarkfonts1.htm
works for me now, and thats also where the version comes from, the soundfont is
called: "PersonalCopy Lite v4r1" on the webpage.

New version with fixed version field here:
Spec URL: http://people.atrpms.net/~hdegoede/PersonalCopy-Lite-soundfont.spec
SRPM URL:
http://people.atrpms.net/~hdegoede/PersonalCopy-Lite-soundfont-4.1-3.fc9.src.rpm
Comment 23 Mamoru TASAKA 2008-03-04 02:30:23 EST
Well the upstream URL still seems to be 404 from me (why?), however
it is okay.

---------------------------------------------------------------------------
        This package (PersonalCopy-Lite-soundfont) is
        APPROVED by me
----------------------------------------------------------------------------
Comment 24 Hans de Goede 2008-03-04 03:51:47 EST
(In reply to comment #23)
> Well the upstream URL still seems to be 404 from me (why?)

Its probably some internet routing problem from your location to upstream?

New Package CVS Request
=======================
Package Name:      PersonalCopy-Lite-soundfont
Short Description: Lite version of the PersonalCopy General Midi soundfont
Owners:            jwrdegoede
Branches:          F-7 F-8
InitialCC: 
Cvsextras Commits: Yes
Comment 25 Kevin Fenzi 2008-03-04 12:00:57 EST
cvs done.
Comment 26 Hans de Goede 2008-03-04 14:48:09 EST
Thanks for the review!

Imported and build for rawhide (for now I'll ask Spot if he wants this as an
update to F-7 / F-8 too).

Closing.

Note You need to log in before you can comment on or make changes to this bug.