Bug 191036 - Review Request: libmp4v2 a library for handling the mp4 container format
Review Request: libmp4v2 a library for handling the mp4 container format
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dominik 'Rathann' Mierzejewski
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-05-08 07:00 EDT by Noa Resare
Modified: 2007-11-30 17:11 EST (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-03-23 06:06:31 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
dominik: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Noa Resare 2006-05-08 07:00:57 EDT
Spec URL: http://resare.com/noa/fedora-extras/libmp4v2.spec
SRPM URL: http://resare.com/noa/fedora-extras/libmp4v2-1.4.1-1.src.rpm
Description: The libmp4v2 library provides an abstraction layer for working with files using the mp4 container format. This library is developed by mpeg4ip project and is an exact copy of the library distributed in the mpeg4ip package.

This is my first extras package, and I'm looking for a sponsor.
Comment 1 Michael A. Peters 2006-05-08 07:47:34 EDT
I can't sponsor, but some notes:

---
%files
%doc COPYING
%defattr(-,root,root,-)
%{_bindir}/*
%{_libdir}/*.so.*
---

Normally - %doc goes below %defattr line

---
%files devel
%doc README TODO INTERNALS API_CHANGES
%doc %{_mandir}/man?/*
%{_includedir}/*.h
%{_libdir}/*.a
%{_libdir}/*.so
---

You nead a defattr line here.
Don't use %doc with %{_mandir} - it isn't necessary.
Don't package the static library (unless you know you need it)

----
I see shared libraries - but don't see ldconfig run in %post or %postun.

Changelog version refers to lvn - it should just be

1.4.1-1
Comment 2 Michael A. Peters 2006-05-08 07:53:03 EDT
On another note - this package will conflict with faad2 from rpm.livna.org

Comment 3 Noa Resare 2006-05-08 08:24:07 EDT
Thanks an updated version is posted at

http://resare.com/noa/fedora-extras/libmp4v2-1.4.1-2.src.rpm
http://resare.com/noa/fedora-extras/libmp4v2.spec

I know that this conflicts with livna. Including this in extras i part of my
grand plan to get an updated faad2 into livna (newer faad2 lacks the libmp4v2
component)
Comment 4 Ville Skyttä 2006-05-08 10:49:51 EDT
I'm afraid this is something that will need an ack from someone (legal?) who can
tell if it's acceptable in FE.  FWIW, to me it looks like a "no".  For example,
from http://mpeg4ip.sourceforge.net/faq/index.php

Q: What are the licensing terms associated with this project?
A:  Like most modern codecs, MPEG-4 Video and Audio codecs are almost certainly
subject to patent royalities. This project does not remove any responsiblity or
liability from developers or users of this kit.  [...]
Comment 5 Noa Resare 2006-05-08 10:56:32 EDT
I've sent this message to legal@fedoraproject.org now:

I would like to include libmp4v2 into Fedora Extras. It is a library
that provides an interface to the mp4 container format. Once included it
can be used to provide mp4/aac support to for example easytag.

So, my question to the fedora project is this: can fedora extras accept
the contribution from a legal standpoint? On one hand the libmp4v2
package has no knowledge about the actual audio and video coding
algorithms that are a part of the mpeg 4 standard. That would make it
immune to the patent issues surrounding mpeg4 that I'm aware of. On the
other hand the patent situation with regards to mpeg 4 is unclear at
best.

There is some precedent in the form of the id3lib package, that provides
similiar functionality but is a little bit more narrow in it's scope.
Comment 6 Noa Resare 2006-05-29 05:45:40 EDT
While waiting for a word from the legal gurus, libmp4v2 is available from
rpm.livna.org
Comment 7 Matthias Saou 2006-07-11 13:05:09 EDT
FWIW, mpeg4ip 1.5.0.1 has been released.

Is there any effort going on to try and get upstream to officially split out
libmp4v2 from mpeg4ip, so that the whole mess between mpeg4ip and faad2 both
providing it can be fixed? It seems like this is what you're trying to do for
Fedora packages, but it would definitely benefit to other people and
distributions if done upstream directly. 
Comment 8 Noa Resare 2006-07-18 07:03:37 EDT
The effort was nonexistant until five minutes ago when I sent an email to the
upstream maintainer. Thanks for the suggestion. I'll add any new info here.
Comment 9 Kevin Fenzi 2006-09-02 01:23:28 EDT
Removing FE-NEEDSPONSOR as submitter was sponsored in: 
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=191592
Comment 10 Matthias Saou 2006-09-22 12:43:28 EDT
Any new info?
Comment 12 Tom "spot" Callaway 2006-10-14 17:45:03 EDT
Without the codec functionality, and with the id3lib precedence, I think this is
OK. Lifted FE-Legal.
Comment 13 Tom "spot" Callaway 2006-10-14 18:33:54 EDT
Scratch that. It's demultiplexing (and multiplexing) the mp4 format. This might
be patented, setting back to FE-Legal for a Patent lawyer to check.
Comment 14 Tom "spot" Callaway 2006-12-05 13:50:38 EST
Lifted FE-Legal here, after discussion with Max. I cannot find any patents on
multiplexing/demuxing in software.
Comment 15 Matthias Saou 2006-12-12 07:08:47 EST
Great, if we can have this in Extras.

First review steps, the spec file :
- Please don't mix space and tab identing in spec files (there is one tab).
- I suggest you use %{version} in Source0 line to be sure not to miss an update.
- You should update to your 1.5.0.1 package, I guess...
- Do we want to be shipping the static library? Probably not.
- You should switch to "make install DESTDIR=$RPM_BUILD_ROOT" (it works).

About the build :
- It compiles fine (tested FC6 x86_64), but with quite a few warnings.

About the resulting packages :
I tried to recompile the latest easytag, enabling libmp4v2 support, but the
build failed with this error :

gcc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m64 -mtune=generic -I/usr/include/gtk-2.0
-I/usr/lib64/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo
-I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include
-I/usr/include/freetype2 -I/usr/include/libpng12 -o easytag about.o ape_tag.o
bar.o browser.o cddb.o charset.o crc32.o dlm.o easytag.o et_core.o flac_header.o
flac_tag.o id3_tag.o misc.o monkeyaudio_header.o mpeg_header.o mp4_header.o
mp4_tag.o musepack_header.o msgbox.o ogg_header.o ogg_tag.o picture.o prefs.o
scan.o setting.o vcedit.o  -L/lib64 -lgtk-x11-2.0 -lgdk-x11-2.0 -latk-1.0
-lgdk_pixbuf-2.0 -lpangocairo-1.0 -lpango-1.0 -lcairo -lgobject-2.0
-lgmodule-2.0 -ldl -lglib-2.0 libapetag/libapetag.a id3lib/libid3bugfix.a
-lmp4v2 -lz -lstdc++ -lid3 -lFLAC -lvorbisfile -lvorbis -logg -lm
mp4_header.o: In function `getType':
/usr/src/rpm/BUILD/easytag-1.99.13/src/mp4_header.c:125: undefined reference to
`MP4GetTrackMediaDataName'
mp4_header.o: In function `Mp4_Header_Read_File_Info':
/usr/src/rpm/BUILD/easytag-1.99.13/src/mp4_header.c:240: undefined reference to
`MP4GetTrackAudioChannels'
collect2: ld returned 1 exit status

The linking stage seems to be including -lmp4v2 properly, but fails to find
those two functions (using 1.4.1). Do you have any idea why? Would it be a
problem with libmp4v2 or with how easytag tries to use it?
Comment 16 Noa Resare 2006-12-12 09:30:10 EST
I'm sorry to say that I won't have time to pick this up in reasonable time. I
had some hopes that when my employment situation changed a month ago I would get
some time to contribute to FE, but it seems like being the proud employee of a
two person startup doesn't give that much free time.

Feel free to carry on with the contribution as you see fit Matthias.
Comment 17 Matthias Saou 2006-12-18 13:48:49 EST
OK, I'll pick it up. I've tried the latest 1.5.0.1 which seems to work fine with
all the packages I have over at freshrpms.net, so I'll submit a package of that
version to this bug report once I'm 100% sure all is working (haven't tried
easytag again with that version yet).

At that point, someone will have to reassign the but to themselves in order to
do the formal review, since I can't review a package which will be my own
(obviously).
Comment 18 Dominik 'Rathann' Mierzejewski 2006-12-18 14:04:50 EST
I will do the review.
Comment 19 Matthias Saou 2007-02-02 08:37:19 EST
Sorry for taking so long...
I've been using this package for over a month now, and all of the multimedia
package I maintain which make use of libmp4v2 rebuild and run fine against it.

Ready for review :
http://ftp.es6.freshrpms.net/tmp/extras/libmp4v2.spec
http://ftp.es6.freshrpms.net/tmp/extras/libmp4v2-1.5.0.1-3.fc6.src.rpm

Since this bug is currently assigned to me, I'll reassign to you :-)
Comment 20 Dominik 'Rathann' Mierzejewski 2007-02-03 08:25:19 EST
Hey, no problem, I've been busy, too. Review will follow soon.
Comment 21 Dominik 'Rathann' Mierzejewski 2007-02-04 09:53:38 EST
 1. package meets naming and packaging guidelines.
 2. specfile is properly named, is cleanly written and uses macros consistently.
 3. dist tag is present.
 4. build root is correct, but not the recommended version
    -> please add %(%{__id_u} -n) at the end before importing
 5. license field matches the actual license.
 6. license is open source-compatible (MPL 1.1). License text included in package.

    However, the following file has no license header:
    libmp4v2-1.5.0.1/atom_ohdr.cpp
    Please poke upstream to fix that.

 7. source files match upstream:
    90eb2b0940ebe02ef81b7a60530beaee  libmp4v2-1.5.0.1.tar.bz2
    4b4abb862b079a7e296c891d96faebc9  mklibmp4v2-r51.tar.bz2
 8. latest version is being packaged.
 9. BuildRequires are proper.
10. package builds in mock (fc7/x86_64).
11. rpmlint is silent.
12. final provides and requires are sane:

    libmp4v2 = 1.5.0.1-3.fc7
    =
    /sbin/ldconfig  
    libc.so.6()(64bit)  
    libgcc_s.so.1()(64bit)  
    libm.so.6()(64bit)  
    libmp4v2.so.0()(64bit)  
    libstdc++.so.6()(64bit)  

    libmp4v2-devel = 1.5.0.1-3.fc7
    =
    libmp4v2 = 1.5.0.1-3.fc7
    libmp4v2.so.0()(64bit)  

13. shared libraries are present and ldconfig is called in %post(un).
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. %check is not present, no testsuite.
21. no scriptlets present.
22. code, not content.
23. documentation is small, so no -docs subpackage is necessary.
24. %docs are not necessary for the proper functioning of the package.
25. headers present in -devel package only.
26. no pkgconfig files.
27. no libtool .la droppings.
28. not a GUI app.
29. not a web app.

4. and 6. need work, but are not blockers, so
APPROVED
Comment 22 Dominik 'Rathann' Mierzejewski 2007-03-22 17:11:47 EDT
Why is this still not imported and built?
Comment 23 Dominik 'Rathann' Mierzejewski 2007-03-22 18:01:09 EDT
Or rather, why is this bug not closed, since it has already been built.
Comment 24 Matthias Saou 2007-03-23 06:06:31 EDT
Simply because I missed it. Since I'm not the reported or the assignee, it
doesn't appear in my bugzilla front page...

Closing now at last :-)

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