Bug 221405 - Review Request: dirac - Dirac is an open source video codec
Review Request: dirac - Dirac is an open source video codec
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2007-01-04 10:13 EST by Nicolas Chauvet (kwizart)
Modified: 2007-12-18 21:13 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-03-26 03:41:07 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kwizart: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Nicolas Chauvet (kwizart) 2007-01-04 10:13:20 EST
Spec URL:
http://kwizart.free.fr/fedora/6/testing/libdirac/libdirac.spec
SRPM URL:
http://kwizart.free.fr/fedora/6/testing/libdirac/libdirac-0.6.0-2.cvs20070104.kwizart.fc6.src.rpm
Description: Dirac is an open source video codec 

Additional info: 
- rpmbuild -ba > log:
http://kwizart.free.fr/fedora/6/testing/libdirac/libdirac.log
- rpmlint is silent with both source and binaries.
- never tried with mock... 
- libdirac-4.3.so has a SONAME

Asking the developper (of dirac) for to support this review...

Please also note that i need to be sponsored!
Comment 1 Dominik 'Rathann' Mierzejewski 2007-01-04 12:10:42 EST
I'm not a sponsor, but I can do a non-binding review.
Comment 2 Dominik 'Rathann' Mierzejewski 2007-01-04 12:11:10 EST
I'm not a sponsor, but I can do a non-binding review.
Comment 3 Ralf Corsepius 2007-01-04 13:25:11 EST
Some initial remarks:

- Don't build the static libs (Append --disable-static to %configure).

- Don't run the autotools as part of building. Autotool-generated files should
  be part of the source code. (Running the autotools as part of building 
  exposes a package to subtile incompatiblities between different versions 
  of the autotools. In fact, this package really suffers from some (however
  minor) issues).

- The package doesn't honour RPM_OPT_FLAGS correctly: It plays dirty games 
  on CFLAGS/CXXFLAGS (E.g. it must not append -O3 and -mmx).

- The *-devel subpackage ships a *.pc. Therefore it must 
Requires: pkgconfig
Comment 4 Nicolas Chauvet (kwizart) 2007-01-04 15:18:26 EST
The release 3 will come soon...

1- OK for now (but if a program need to link with the static version?)
   (.la and .a removed from the package)
2- KO - Don't know how to do since ./configure will not be found.
3- OK
4- OK
(5)- I've also add some BR (mock tries!)
(6)- doxygen -u when required!

I wonder these message: (install step)
libtool: install: warning: remember to run `libtool --finish /usr/lib64'
Should i do it do it myself ? (libtool is now in BR!)
Comment 5 Nicolas Chauvet (kwizart) 2007-01-04 17:37:50 EST
Release 3:
Spec URL:
http://kwizart.free.fr/fedora/6/testing/libdirac/libdirac.spec
SRPM URL:
http://kwizart.free.fr/fedora/6/testing/libdirac/libdirac-0.6.0-3.cvs20070104.kwizart.fc6.src.rpm
Description: Dirac is an open source video codec 
Build Log: http://kwizart.free.fr/fedora/6/testing/libdirac/libdirac.log

2 - OK understood, the bootstrap in included in the diract-snapshoot.sh
(but should it use direct url to download the package with next snapshoot? for
exemple on my website?
Comment 6 Ralf Corsepius 2007-01-05 08:00:25 EST
- *-3 still doesn't honour RPM_OPT_FLAGS and still plays dirty games with
CFLAGS/CXXFLAGS. /me thinks you misunderstood my earlier remark.

The problem is not "make RPM_OPT_FLAGS="$RPM_OPT_FLAGS"" (Which doen't make any
sense), but the configure script mis-handling CFLAGS/CXXFLAGS and appending
compiler flags to them it MUST not add to them, such as "-O3" and "-mmmx".

- Why the dependency on valgrind?
AFAIS, this package only uses valgrind in some part of its testsuite, but not as
part of building. I.e. this dependency on valgrind only makes sense if your are
running the testsuite as part of building.

- Instead of directly using a cvs snapshot, you'd better base your package on a
released version and apply a patch if you really think shipping a bleeding edge
version is useful (I don't know, I am not familiar with libdirac's stability).

- Finally, the "autotool" correct way to generate a cvs snapshot-tarball would
be to run
cvs co && autoreconf -fi && ./configure && make dist
Comment 7 Nicolas Chauvet (kwizart) 2007-01-05 10:33:53 EST
-1 (old 3) Don't know much about compile flag but since diract in a codec,
compiling it with -mmx seems not too much!? (searching for aims of -03...) I
expect it deals with i586/i686 ? Since it is an encoder and requires hight
performances (to encode and certainly also to decode), i wonder if it is not
better to stay with it?
Tryed do do the changes but this seems not to work (searching...wip):
%configure --enable-overlay --disable-static CXXFLAGS="${RPM_OPT_FLAGS}"
CFLAGS="${RPM_OPT_FLAGS}"

2 OK (commented...)

3 The project is now involved in the hardware implementation. Encode/Decode
Software "should" work! There is some enancement to wait
http://dirac.sourceforge.net/todo.html! ... I have browsed the CVS and the last
commit was 4 weeks ago. (the last release is 0.6.0 and was out 6 months ago)
I cannot see a better time to make a snapshot ? what do you feel?

4 OK (added to diract-snapshot and updated to 20070105)
Comment 8 Marcin Garski 2007-01-05 16:20:10 EST
(In reply to comment #7)
> -1 (old 3) Don't know much about compile flag but since diract in a codec,
> compiling it with -mmx seems not too much!?

Nicolas the problem lays in configure.ac file, which is used to generate
configure and later Makefile. If you view this file and jump to "enable debug
flags" and "enable MMX optimization flag" section, you will notice that the
first one sets -O3 and the second -mmmx.
Probably these sections needs to be clean up, then you have to do autoreconf to
generate updated configuration files.
Comment 9 Nicolas Chauvet (kwizart) 2007-01-08 14:40:45 EST
SPEC :
http://kwizart.free.fr/fedora/6/testing/libdirac/libdirac.spec
SRPMS:
http://kwizart.free.fr/fedora/6/testing/libdirac/libdirac-0.6.0-5.cvs20070108.kwizart.fc6.src.rpm
Description:
Dirac is an open source video codec 
Build Log: (2> .log)
http://kwizart.free.fr/fedora/6/testing/libdirac/libdirac-0.6.0-5.log

It seems that using configure can disable theses unwanted FLAGs without
autoreconf. I think it is a better solution.
I wonder why the src.rpm has grown up. Since i was using make tag, the produced
release dirac-0.6.0.tar.gz was bundled with the sources but no used.
The only thing that wasn't bundled in this version was the qt4 gui encoder. But
anyway it can be renabled later...

I should have : (but since it is not release...)
provides: dirac-qt4
obsolete: dirac-qt4 < %{version}-5.cvs20070108

The only rpmlint error i have is :
E: libdirac configure-without-libdir-spec
A configure script is run without specifying the libdir. configure
options must be augmented with something like --libdir=%{_libdir}.

But the %configure script bundle it so i wonder if it is not a problem with the
./configure in the snapshot step 
Comment 10 Dominik 'Rathann' Mierzejewski 2007-01-20 17:23:21 EST
Per
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-cfd71146dbb6f00cec9fe3623ea619f843394837
the release field should be:
Release:        5.%{cvs}cvs%{?dist}

Why is the testsuite disabled?

I don't like /usr/bin/ binaries in a package called libwhatever. Moreover, if
the source tarball is called dirac, why is the main package called libdirac?
Please put the binaries in a package named dirac

There are two redundant BuildRequires: libtool requires automake and automake
requires autoconf.

Builds fine in mock/x86_64/devel.

Shouldn't package docs be in %{_datadir}/doc/dirac-docs-%{version} ?
Comment 11 Nicolas Chauvet (kwizart) 2007-01-20 21:35:58 EST
SRPM:
http://kwizart.free.fr/fedora/6/testing/dirac/dirac-0.6.0-6.20070108cvs.kwizart.fc6.src.rpm
SPEC:
http://kwizart.free.fr/fedora/6/testing/dirac/dirac.spec
Description: Dirac is an open source video codec 

Test-suite is not provided with this snapshoot scheme (done from remark in #6 )
same problem with qt4-encodeur gui (anyway i'm not sure we need an encoder gui
for each codec !?). if encoder_gui and test-suite are really needed the best way
to do this would be to make another snaphoot and a tarball with only theses two
wanted parts!

Encoding seems to work fine but isn't useable since playbak need to be tested...

Comment 12 Dominik 'Rathann' Mierzejewski 2007-01-28 17:05:24 EST
Looking good now.

Small issue: it is preferable to ship the desktop file separately instead of
creating it in %install.

These files have names that are IMHO too generic. Maybe rename them to dirac_*?
%{_bindir}/BMPtoRGB                                                            
                                        
%{_bindir}/RGBtoBMP                                                            
                                        
%{_bindir}/RGBtoUYVY                                                           
                                        
%{_bindir}/RGBtoYUV*                                                           
                                        
%{_bindir}/UYVYtoRGB                                                           
                                        
%{_bindir}/YUV*toRGB
Comment 13 Mamoru TASAKA 2007-03-01 13:13:47 EST
(Removing NEEDSPONSOR by bug 225119)
Comment 14 Dominik 'Rathann' Mierzejewski 2007-03-22 19:18:05 EDT
 1. package mostly meets naming and packaging guidelines.
    -> See comment #12.
    -> You could leave MMX optimizations enabled on x86_64 (assuming they work)
 2. specfile is properly named, is mostly cleanly written and uses macros
consistently.
    -> specfile contains a lot of commented-out code, please remove those parts
 3. dist tag is present.
 4. build root is sane.
 5. license field matches the actual license.
 6. license is open source-compatible (MPL). License text included in package.
 7. source files match upstream.
    Unable to verify md5sum because it's a snapshot, but diff -Nru between
    packaged tree and one checked out locally using the included script
    shows no differences.
 8. latest version is not being packaged, but I think it should be left
    at packager's discretion, as he should know which snapshot is stable.
 9. BuildRequires are proper.
10. package builds in mock (fc6/x86_64).
11. rpmlint is not silent, but these can be ignored:
    W: dirac-devel no-documentation
    E: dirac configure-without-libdir-spec
12. final provides and requires are sane:

dirac-0.6.0-6.20070108cvs.fc6.x86_64.rpm
libdirac_decoder.so.0()(64bit)  
libdirac_encoder.so.0()(64bit)  
dirac = 0.6.0-6.20070108cvs.fc6
=
/sbin/ldconfig  
/usr/bin/perl  
libc.so.6()(64bit)  
libdirac_decoder.so.0()(64bit)  
libdirac_encoder.so.0()(64bit)  
libgcc_s.so.1()(64bit)  
libm.so.6()(64bit)  
libstdc++.so.6()(64bit)  
perl(File::Basename)  
perl(Getopt::Long)  
perl(strict)  

dirac-devel-0.6.0-6.20070108cvs.fc6.x86_64.rpm
dirac-devel = 0.6.0-6.20070108cvs.fc6
=
dirac = 0.6.0-6.20070108cvs.fc6
libdirac_decoder.so.0()(64bit)  
libdirac_encoder.so.0()(64bit)  
pkgconfig  

13. shared libraries are present and handled properly
14. package is not relocatable.
15. owns the directories it creates.
16. doesn't own any directories it shouldn't.
17. no duplicates in %files.
18. file permissions are appropriate.
19. %clean is present.
20. test suite not included with snapshot, hence no %check
21. proper %post/%postun scriptlets present.
22. code, not content.
23. documentation is not small, -docs subpackage present.
24. %docs are not necessary for the proper functioning of the package.
25. headers in -devel
26. pkgconfig file in -devel, pkgconfig in Requires:
27. no libtool .la droppings.
28. GUI is not built/shipped, hence no .desktop file is necessary.
29. not a web app.

NEEDSWORK: 1, 2.
Comment 15 Nicolas Chauvet (kwizart) 2007-03-24 18:08:29 EDT
SRPM:
http://kwizart.free.fr/fedora/6/testing/dirac/dirac-0.6.0-7.20070108cvs.kwizart.fc6.src.rpm
SPEC:
http://kwizart.free.fr/fedora/6/testing/dirac/dirac.spec
Description: Dirac is an open source video codec 

Fixed 1 and 2

I've tryed to test in a same directory a Test_Video.yuv in YUV (analogic source
from tv tuner), encoding it to drc (Test1.drc) and then decoding it to yuv
(Test1.yuv)
I didn't get then resulting Test1.yuv playing. I was using dirac_encoder and
dirac_decoder tools from theses packages...

I will try to do more tests...

The videolan project seems not to support dirac 0.6 (prefered version seems to
be 5.4 ). They are trying to support it throught ffmpeg. (discution on irc)


Comment 16 Dominik 'Rathann' Mierzejewski 2007-03-25 11:51:48 EDT
Ekhm...

%ifarch x86_64 ppc64 sparc64 \
       --enable-mmx=yes \
%else \
       --enable-mmx=no \
%endif

MMX should be enabled unconditionally for x86_64 ONLY.
Comment 17 Nicolas Chauvet (kwizart) 2007-03-25 12:11:06 EDT
SRPM:
http://kwizart.free.fr/fedora/6/testing/dirac/dirac-0.6.0-8.20070108cvs.kwizart.fc6.src.rpm
SPEC:
http://kwizart.free.fr/fedora/6/testing/dirac/dirac.spec
Description: Dirac is an open source video codec 

Fixed
yes actually i didn't know it was only for x86 intel or amd. i've only know that
some via cpu didn't use mmx so that it needs to be removed for x86.
Comment 18 Dominik 'Rathann' Mierzejewski 2007-03-25 12:13:39 EDT
New rpmlint errors:
E: dirac-libs library-without-ldconfig-postin /usr/lib64/libdirac_encoder.so.0.0.0
E: dirac-libs library-without-ldconfig-postun /usr/lib64/libdirac_encoder.so.0.0.0
E: dirac-libs library-without-ldconfig-postin /usr/lib64/libdirac_decoder.so.0.0.0
E: dirac-libs library-without-ldconfig-postun /usr/lib64/libdirac_decoder.so.0.0.0
W: dirac mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line 73)

Looks like you forgot to move ldconfig calls to -libs, i.e.:
Comment 19 Dominik 'Rathann' Mierzejewski 2007-03-25 12:15:40 EDT
...
%post libs -p /sbin/ldconfig
%postun libs -p /sbin/ldconfig

(I hit send too quickly)
Comment 20 Nicolas Chauvet (kwizart) 2007-03-25 12:22:58 EDT
SRPM:
http://kwizart.free.fr/fedora/6/testing/dirac/dirac-0.6.0-8.20070108cvs.kwizart.fc6.src.rpm
SPEC:
http://kwizart.free.fr/fedora/6/testing/dirac/dirac.spec
Description: Dirac is an open source video codec 

Yes just saw that too - This is fixed with release 8
Comment 22 Dominik 'Rathann' Mierzejewski 2007-03-25 19:37:34 EDT
Looks fine. This package is APPROVED.
Comment 23 Nicolas Chauvet (kwizart) 2007-03-25 19:52:50 EDT
New Package CVS Request
=======================
Package Name: dirac
Short Description: Dirac is an open source video codec 
Owners: kwizart@gmail.com
Branches: FC-5 FC-6 devel
InitialCC: 
Comment 24 Dennis Gilmore 2007-03-25 21:14:19 EDT
branched
Comment 25 Nicolas Chauvet (kwizart) 2007-12-18 12:10:53 EST
Package Change Request
======================
Package Name: dirac
New Branches: EL-4 EL-5
Comment 26 Nicolas Chauvet (kwizart) 2007-12-18 12:13:39 EST
I'v made some mistake with request Flags here!
Comment 27 Kevin Fenzi 2007-12-18 21:13:22 EST
cvs done.

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