Bug 904658 - Review Request: radium-compressor: an audio compressor for JACK
Review Request: radium-compressor: an audio compressor for JACK
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FedoraAudio
  Show dependency treegraph
 
Reported: 2013-01-26 16:41 EST by Brendan Jones
Modified: 2013-02-28 02:02 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-02-17 13:09:14 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
mtasaka: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Brendan Jones 2013-01-26 16:41:48 EST
Radium-compressor is a standalone audio compressor for use with JACK. It features an intuitive UI using the Qt toolkit and uses Faust generated classes to handle DSP.

SPEC: http://bsjones.fedorapeople.org/reviews/radium-compressor.spec
SRPM: http://bsjones.fedorapeople.org/reviews/radium-compressor-0.5.1-1.fc18.src.rpm

rpmlint /home/bsjones/rpmbuild/SRPMS/radium-compressor-0.5.1-1.fc18.src.rpm /home/bsjones/rpmbuild/RPMS/x86_64/radium-compressor-0.5.1-1.fc18.x86_64.rpm ~/rpmbuild/SPECS/radium-compressor.spec 
radium-compressor.x86_64: W: no-manual-page-for-binary radium_compressor
2 packages and 1 specfiles checked; 0 errors, 1 warnings.
Comment 1 Mamoru TASAKA 2013-02-02 12:46:16 EST
For 0.5.1-1

* License
  - Some files are under GPLv3+, and some of them are actually
    used to build the binary:
-----------------------------------------------------
audio/faudiostream/architecture/faust/audio/alsa-dsp.h
audio/faudiostream/architecture/faust/audio/coreaudio-dsp.h
audio/faudiostream/architecture/faust/audio/netjack-dsp.h
audio/faudiostream/architecture/faust/audio/portaudio-dsp.h
audio/faudiostream/architecture/faust/gui/console.h
audio/faudiostream/architecture/faust/gui/faustqt.h
audio/faudiostream/architecture/faust/gui/meta.h
faudiostream/architecture/faust/audio/alsa-dsp.h
faudiostream/architecture/faust/audio/coreaudio-dsp.h
faudiostream/architecture/faust/audio/netjack-dsp.h
faudiostream/architecture/faust/audio/portaudio-dsp.h
faudiostream/architecture/faust/gui/console.h
faudiostream/architecture/faust/gui/faustqt.h
faudiostream/architecture/faust/gui/meta.h
-----------------------------------------------------
    So the license tag should be "GPLv3+" (just GPLv3+
    is okay).

* Compiler flags
  - Fedora specific compilation flags are not used
    correctly. build.log says:
-----------------------------------------------------
    33  Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.ESDXyl
    34  + umask 022
    35  + cd /builddir/build/BUILD
    36  + cd radium_compressor-0.5.1
    37  + make 'OPTS=-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4  -m32 -march=i686 -mtune=atom -fasynchronous-unwind-tables' -j5
    38  cd Qt && ./create_source_from_ui.sh `../find_moc_and_uic_paths.sh uic` `../find_moc_and_uic_paths.sh moc` compressor_widget
    39  g++ -DDEBUG -O3 -Wall -msse -mfpmath=sse -DUSE_QT_REQTYPE -DUSE_QT4 -g -I. -ffast-math -IQt Qt/Qt_SliderPainter.cpp `pkg-config --cflags Qt3Support` -c
    40  g++ -DDEBUG -O3 -Wall -msse -mfpmath=sse -DUSE_QT_REQTYPE -DUSE_QT4 -g -I. -ffast-math -IQt main.cpp Qt_SliderPainter.o -Iaudio/faudiostream/architecture/ `pkg-config --libs --cflags Qt3Support` -ljack -o radium_compressor
    41  + exit 0
 -----------------------------------------------------
    Also, "-O3" is used, this should be changed to -O2.
    And, is it safe for this package to use "-ffast-math"?
    -ffast-math does some "optimization" which breaks IEEE rules
    for example, which may result in confusing behavior.

* rpmlint issue
  - incorrect-fsf-address (not a blocker)
    - Some source files contain incorrest FSF address, which causes
      rpmlint complaint on debuginfo rpm. Not a blocker, however it
      is recommend to tell upstream

  - spurious-executable-perm
    - rpmlint complains about some header files in debuginfo rpm for
      having non-proper permission. You can suppress this rpmlint complaint
      by changing the permission of header files to 0644 at %prep.

* Desktop file
  https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Desktop_files
  - It seems that "radium_compressor" is a GUI program, so
    installing proper desktop file is required (unless there is
    some reason, e.g. this program usually needs proper argument,
    for example)
Comment 2 Kjetil Matheussen 2013-02-02 16:39:08 EST
As far as I can read from http://gcc.gnu.org/onlinedocs/gcc-4.7.0/gcc/Optimize-Options.html, -ffast-math should be safe, and benchmarking showed that -ffast-math makes the program run 13% faster.

(I'm the upstream author)
Comment 3 Kjetil Matheussen 2013-02-02 17:18:13 EST
Oh, I also see that -O3 makes the program run twice as fast as -O2 (at least on my computer): make benchmark && ./benchmark. It's -fpredictive-commoning and -ftree-vectorize that makes the difference. "-O2 -fpredictive-commoning -ftree-vectorize" runs at the same speed as "-O3".
Comment 4 Mamoru TASAKA 2013-02-02 22:28:45 EST
(In reply to comment #3)
> Oh, I also see that -O3 makes the program run twice as fast as -O2 (at least
> on my computer): make benchmark && ./benchmark. It's -fpredictive-commoning
> and -ftree-vectorize that makes the difference. "-O2 -fpredictive-commoning
> -ftree-vectorize" runs at the same speed as "-O3".

Well, -O2 vs -O3 is rather Fedora packaging (policy) issue. I can accept "-O2 -fpredictive-commoning -ftree-vectorize" instead.
Comment 5 Mamoru TASAKA 2013-02-02 23:01:43 EST
(In reply to comment #3)
> Oh, I also see that -O3 makes the program run twice as fast as -O2 (at least
> on my computer): make benchmark && ./benchmark. It's -fpredictive-commoning
> and -ftree-vectorize that makes the difference. "-O2 -fpredictive-commoning
> -ftree-vectorize" runs at the same speed as "-O3".

Oh, https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Compiler_flags
says 
"Overriding these flags for performance optimizations (for instance, -O3 instead of -O2) is generally discouraged. If you can present benchmarks that show a significant speedup for this particular code, this could be revisited on a case-by-case basis. "

So as you kindly presented benckmarks, I can accept -O3 and -ffast-math.
Still other Fedora specific compilation flags (such as -Wp,-D_FORTIFY_SOURCE=2 and so on) must be honored.
Comment 6 Brendan Jones 2013-02-09 03:01:07 EST
Thanks guys for the throguh investigation of the compiler flags. Really appreciated.

Kjetil: there are some incorrect FSF addresses in your source headers (correct is 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA). You can update this at your disgression

License has been ajusted and file permissions addressed. 

I have added the desktop file and will liase with upstream regarding a suitable icon.

SPEC: http://bsjones.fedorapeople.org/reviews/radium-compressor.spec
SRPM: http://bsjones.fedorapeople.org/reviews/radium-compressor-0.5.1-2.fc18.src.rpm
Comment 7 Mamoru TASAKA 2013-02-10 11:19:26 EST
Some notes:

* Optimization level
  - For changing optimization level, please add some comments on
    your spec file about the discussion on this review request
    (especially the comments / explanation from Kjetil)
    for justification.

! Honoring Fedora specific compilation flags
  - Well, it seems somewhat strange that you have to use sed for Makefile
    and also change "OPTS" variables to honor Fedora specific compilation
    flags, however not a blocker.

-------------------------------------------------------------
   This package (radium-compressor) is APPROVED by mtasaka
-------------------------------------------------------------
Comment 8 Brendan Jones 2013-02-11 15:51:38 EST
(In reply to comment #7)
> Some notes:
> 
> * Optimization level
>   - For changing optimization level, please add some comments on
>     your spec file about the discussion on this review request
>     (especially the comments / explanation from Kjetil)
>     for justification.

Good call - will do.

> 
> ! Honoring Fedora specific compilation flags
>   - Well, it seems somewhat strange that you have to use sed for Makefile
>     and also change "OPTS" variables to honor Fedora specific compilation
>     flags, however not a blocker.
> 

I think this is a feature in the build script. I'll send a patch upstream.

Thanks for the review!
Comment 9 Brendan Jones 2013-02-11 15:52:32 EST
New Package SCM Request
=======================
Package Name: radium-compressor
Short Description: An audio compressor for JACK
Owners: bsjones
Branches: f17 f18
Comment 10 Jon Ciesla 2013-02-12 08:29:39 EST
Git done (by process-git-requests).
Comment 11 Fedora Update System 2013-02-13 06:40:41 EST
radium-compressor-0.5.1-3.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/radium-compressor-0.5.1-3.fc17
Comment 12 Fedora Update System 2013-02-13 06:41:04 EST
radium-compressor-0.5.1-3.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/radium-compressor-0.5.1-3.fc18
Comment 13 Fedora Update System 2013-02-13 21:25:38 EST
radium-compressor-0.5.1-3.fc17 has been pushed to the Fedora 17 testing repository.
Comment 14 Fedora Update System 2013-02-14 12:23:46 EST
radium-compressor-0.5.1-4.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/radium-compressor-0.5.1-4.fc17
Comment 15 Fedora Update System 2013-02-14 12:24:02 EST
radium-compressor-0.5.1-4.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/radium-compressor-0.5.1-4.fc18
Comment 16 Mamoru TASAKA 2013-02-17 13:09:14 EST
Closing.
Comment 17 Fedora Update System 2013-02-28 01:57:12 EST
radium-compressor-0.5.1-4.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 18 Fedora Update System 2013-02-28 02:02:42 EST
radium-compressor-0.5.1-4.fc18 has been pushed to the Fedora 18 stable repository.  If problems still persist, please make note of it in this bug report.

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