Bug 248119 - Review Request: libtimidity - MIDI to WAVE converter library
Review Request: libtimidity - MIDI to WAVE converter library
Status: CLOSED NEXTRELEASE
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:
Blocks:
  Show dependency treegraph
 
Reported: 2007-07-13 08:00 EDT by Hans de Goede
Modified: 2007-11-30 17:12 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-07-24 14:58:52 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Hans de Goede 2007-07-13 08:00:43 EDT
Spec URL: http://people.atrpms.net/~hdegoede/libtimidity.spec
SRPM URL: http://people.atrpms.net/~hdegoede/libtimidity-0.1.0-1.fc8.src.rpm
Description:
This library is based on the TiMidity decoder from SDL_sound library.
Purpose to create this library is to avoid unnecessary dependences.
SDL_sound requires SDL and some other libraries, that not needed to
process MIDI files. In addition libtimidity provides more suitable
API to work with MIDI songs, it enables to specify full path to the
timidity configuration file, and have function to retrieve meta data
from MIDI song.


---

Reviewers: note, this is necessary to add midi playback support to gstreamer, since the plugin is currently under development it is in gstreamer-plugins-bad from the repo that must not be named. To test this, install libtimidity-devel and the gstreamer-plugins-bad.src.rpm and rebuild gstreamer-plugins-bad.
Comment 1 Mamoru TASAKA 2007-07-23 10:57:59 EDT
For 0.1.0-1:

* License
  - While COPYING says this is licensed under LGPL (and you
    tagged this rpm as such, although COPYING is missing on
    %doc entry), common.c, instrum.c, etc... are licensed 
    under _GPL_ .

? Dependency
  - Would you explain why main package should explicitly have
    "Requires: timidity++"?

? binary executable files
  - Are test/.libs/{playmidi,midi2raw} of no use?
    (playmidi seems to be created only with have libao-devel
     installed).

* libtimidity.pc
  - Why does this need "-lm" on Libs? Linkage against libm.so
    is already done on libtimidity.so and extra linkage like
    this must be removed unless header files installed require
    it.
Comment 2 Hans de Goede 2007-07-23 11:18:15 EDT
(In reply to comment #1)
> For 0.1.0-1:
> 
> * License
>   - While COPYING says this is licensed under LGPL (and you
>     tagged this rpm as such, although COPYING is missing on
>     %doc entry), common.c, instrum.c, etc... are licensed 
>     under _GPL_ .
> 

These files come from timidity (through SDL_mixer, which also is LGPL), when
timidity was discontinued it was set free under a choice if GPL / LGPL /
artistic, see:
http://web.archive.org/web/20010106150700/http://www.cgs.fi/~tt/discontinued.html

Also see the first paragraph of the included COPYING which says:

Please note that the included source from TiMidity, is also licensed
 under the following terms (GNU LGPL), but can also be used separately
 under the GNU GPL, or the Perl Artistic License. Those licensing
 terms are not reprinted here, but can be found on the web easily.

I will include the missing COPYING file, good catch.

> ? Dependency
>   - Would you explain why main package should explicitly have
>     "Requires: timidity++"?
> 

Because timidity++ provides the necessary patch files containing the wavetable
instruments, this is why for example SDL_mixer and allegro also require timidity++.

> ? binary executable files
>   - Are test/.libs/{playmidi,midi2raw} of no use?
>     (playmidi seems to be created only with have libao-devel
>      installed).
> 

There already is a cmdline utility called playmidi (or atleast was), also we
already have playmus from SDL_mixer and timidity itself through timidity++,
notice that the standalone version of timidity is of much better quality (and
much more cpu intensive). So I see little use in these test apps, if people want
to play or convert midi using timidity they should be using the standalone version.


> * libtimidity.pc
>   - Why does this need "-lm" on Libs? Linkage against libm.so
>     is already done on libtimidity.so and extra linkage like
>     this must be removed unless header files installed require
>     it.

Many package config files contain libs in the LDFLAGS against which the lib
itself is already linked, this is normal behaviour as not all Unix OS's support
.so files having deps, so sometimes the app itself must be linked against the
deps, so that the symbols will be there for the lib.
Comment 3 Mamoru TASAKA 2007-07-23 12:36:35 EDT
All right.
Please add COPYING on CVS.

-------------------------------------------------------
  This package (libtimidity) is APPROVED by me
-------------------------------------------------------
Comment 4 Hans de Goede 2007-07-23 12:51:21 EDT
Thanks for the review!

New Package CVS Request
=======================
Package Name:      libtimidity
Short Description: MIDI to WAVE converter library
Owners:            j.w.r.degoede@hhs.nl
Branches:          FC-6 F-7 devel
InitialCC:         <empty>

Comment 5 Hans de Goede 2007-07-24 14:58:52 EDT
Imported and build, closing.

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