Bug 227309 - Review Request: seom - Desktop video capture and playback utility
Review Request: seom - Desktop video capture and playback utility
Status: CLOSED WONTFIX
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Package Reviews List
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2007-02-04 22:53 EST by Jarod Wilson
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-09-27 09:06:51 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Jarod Wilson 2007-02-04 22:53:42 EST
Spec URL: http://people.redhat.com/jwilson/packages/seom/seom.spec
SRPM URL: http://people.redhat.com/jwilson/packages/seom/seom-1.0-0.1.159.fc7.src.rpm

Description:
Seom is a desktop video capture and playback utility, utilized by the beryl project's vidcap plugin.

NOTE: was originally included as part of beryl-vidcap, but is now a stand-alone entity. The inclusion of this package now blocks beryl-vidcap's approval into extras (bug 215569).
Comment 1 Jarod Wilson 2007-02-04 22:56:49 EST
Builds fine in a rawhide/x86_64 mock chroot, and an updated beryl-vidcap builds against it as well.
Comment 2 Mamoru TASAKA 2007-02-05 10:36:00 EST
Well,

* License
  - No license text exists, and no source code contains license term.
    tomc, do you agree that this is licensed under GPL? 
    If so, please include GPL license document to your tarball from
    next release.

* Requires for -devel package
  - Is "freeglut-devel" for Requires on -devel package
    is needed? This seems to be of no relation for this
    package.

    And according to the header files included, libX11-devel
    is needed for Requires on -devel package. Note that
    libX11-devel automatically pulls mesa-libGL-devel.

* Timestamps
  - Generally keeping timestamps on text files, such as header
    files is preferrred. For this package, this can be done
    by replacing all "install" in Makefile by "install -p".

    And .. libX11-devel requires mesa-libGL-devel finally.
Comment 3 Ralf Corsepius 2007-02-05 10:43:17 EST
(In reply to comment #2)
> * License
IMO, this package is a clear case of an improperly licensed piece SW that can't
be shipped.

Comment 4 Mamoru TASAKA 2007-02-05 10:45:53 EST
(In reply to comment #3)
> (In reply to comment #2)
> > * License
> IMO, this package is a clear case of an improperly licensed piece SW that can't
> be shipped.
> 
Well, upstream is checking this report (perhaps) and so
I will expect he/she posts some comments on this.

Comment 5 Jarod Wilson 2007-02-05 12:05:28 EST
*License-
In bug 215569, Tom said:

----8<----
[...]capture.c has a standard
header just like any other file in beryl-plugins, and the seom files have no
header, just a LICENSE file in the root directory. If you require that every
file needs to have a GPL header, I can add it, no problem.
----8<----

However, said GPL LICENSE file seems to be missing from the seom tarballs...
Tom, a license file alone added to the seom tarball should be sufficient.


* Requires-
Including freeglut-devel is an error on my part, the spec started life as a copy
of beryl-vidcap's. I'll drop that and add libX11-devel.


* Timestamps-
I'll fix that too.
Comment 7 Ralf Corsepius 2007-02-05 12:22:07 EST
> In bug 215569, Tom said:
>
>[...]capture.c has a standard
>header just like any other file in beryl-plugins, and the seom files have no
>header, just a LICENSE file in the root directory.

This does not apply to the seom tarball.

International copyright laws require him to claim ownership (Copyright/Author
disclaimer - Otherwise he can't grant a license) and to provide a License. 
How to do so is arguable.


Also, there is a file inside of the tarball (src/codec.c) containing another
person's copyright without license:
 * Copyright 2006, Lasse Reinhold (lar@quicklz.com)
=> Tom is not allowed to claim copyright rsp. re-license this file, unless he
received a license explicitly permitting him to do so from this person.
Comment 8 Jarod Wilson 2007-02-05 13:05:39 EST
Pretty sure Tom was talking about the seom tarball, and seom is his work, so
claiming ownership to grant a license shouldn't be a problem, assuming the
rights to codec.c can be ironed out. (Hadn't seen that one, yeah, could indeed
be an issue.) Tom?
Comment 9 Mamoru TASAKA 2007-02-07 10:16:38 EST
Well, two issue.

* License:
  - This must be resolved in a proper way.

* /usr/bin/seom-backup
  - is a shell script and this contains
----------------------------------------------------
if ! which seom-x264 &> /dev/null; then
        echo "You need to install seom-x264"
        exit -1
fi
----------------------------------------------------
   .. however, what is seom-x264?
Comment 10 Jarod Wilson 2007-02-07 10:50:46 EST
seom-x264 is a utility to convert seom captures to h.264-formatted video:

http://forum.beryl-project.org/viewtopic.php?f=37&t=1020&start=0

However, I'm fairly certain the build-dep, x264, isn't shippable in Fedora. On
that assumption, should we drop the script from the build, or maybe just drop it
in %docdir non-executable so users can figure it out themselves?

Comment 11 Mamoru TASAKA 2007-02-07 10:54:16 EST
(In reply to comment #10)
> seom-x264 is a utility to convert seom captures to h.264-formatted video:
> 
> http://forum.beryl-project.org/viewtopic.php?f=37&t=1020&start=0
> 
> However, I'm fairly certain the build-dep, x264, isn't shippable in Fedora. On
> that assumption, should we drop the script from the build, or maybe just drop it
> in %docdir non-executable so users can figure it out themselves?
> 

I think this should be dropped.

Comment 12 tomc 2007-02-08 04:36:58 EST
(In reply to comment #2)
> * License

added LICENSE (GPLv2)
btw, quicklz is licensed under the GPL, so that shouldn't be a problem.

> 
> * Timestamps

No reason to add '-p' when installing seom.pc, because that file is
auto-generated. But I added '-p' when installing the headers..


-$(CC) $(CFLAGS) $(LDFLAGS) -L.libs -o $@ src/$@/main.c -lseom $($@LIBS)
+$(CC) $(CFLAGS) $(EXTRA_CFLAGS) $(INCLUDES) $(LDFLAGS) -L.libs -o seom-$@
src/$@/main.c -lseom $($@LIBS)

'seom-$@' is ok in the srpm, but when compiling seom from sources more than once
it's bad because then make will recompile the apps every time make is executed..  

and, what's wrong with 'libtool --mode=install'?
Comment 13 Jarod Wilson 2007-02-08 12:18:32 EST
(In reply to comment #12)
> (In reply to comment #2)
> > * License
> 
> added LICENSE (GPLv2)
> btw, quicklz is licensed under the GPL, so that shouldn't be a problem.

I think we could probably use something along the lines of a README that
includes a bit stating that you're the author of the bulk of the code, with a
reference stating where the codec.c bit came from and that it is also GPL, just
to cover all the bases.

> > 
> > * Timestamps
> 
> No reason to add '-p' when installing seom.pc, because that file is
> auto-generated. But I added '-p' when installing the headers..

Ah, true.

> -$(CC) $(CFLAGS) $(LDFLAGS) -L.libs -o $@ src/$@/main.c -lseom $($@LIBS)
> +$(CC) $(CFLAGS) $(EXTRA_CFLAGS) $(INCLUDES) $(LDFLAGS) -L.libs -o seom-$@
> src/$@/main.c -lseom $($@LIBS)
> 
> 'seom-$@' is ok in the srpm, but when compiling seom from sources more than once
> it's bad because then make will recompile the apps every time make is executed..  

I'll drop that part of my patch.

> and, what's wrong with 'libtool --mode=install'?

For Fedora packages, libtool archives and static libs are forbidden, so it was
solely for the benefit of the package. I'll use a patch that doesn't nuke that,
and will instead remove the file from within the spec.

Other notes on the patch I'm using:
- splitting CFLAGS and EXTRA_CFLAGS makes packaging much easier. Fedora has a
standard set of CFLAGS that are supposed to be used on all packages, so
splitting off the -std=c99 bit into EXTRA_CFLAGS makes life easier, since its
required, but not part of the standard Fedora CFLAGS.
- some of the added $(*DIR) bits are for convenience more than anything, but are
fairly standard.
- you've got lots of extraneous slashes -- $(DESTDIR)/$(PREFIX) works out to
/somewhere//usr -- so I nuke all those.
- I need/want the shared libs, they're greatly preferred over static libs and/or
libtool archives in the Fedora world.


Updated build:

http://people.redhat.com/jwilson/packages/seom/seom-1.0-0.3.161.fc6.src.rpm
Comment 14 tomc 2007-02-08 12:54:49 EST
(In reply to comment #13)
> I think we could probably use something along the lines of a README that
> includes a bit stating that you're the author of the bulk of the code, with a
> reference stating where the codec.c bit came from and that it is also GPL, just
> to cover all the bases.
> 

will do ... I'll drop you an email as soon as a new tarball is available.

> For Fedora packages, libtool archives and static libs are forbidden, so it was
> solely for the benefit of the package. I'll use a patch that doesn't nuke that,
> and will instead remove the file from within the spec.

I would actually love to drop libtool, but it's a nice tool that adds '-fPIC'
when needed (and other arch-dependent flags). But other than that, I find it
horrible (what are these libtool archives for anyway? The compiler doesn't need
them when linking against the library, right?).

> 
> Other notes on the patch I'm using:
> - splitting CFLAGS and EXTRA_CFLAGS makes packaging much easier. Fedora has a
> standard set of CFLAGS that are supposed to be used on all packages, so
> splitting off the -std=c99 bit into EXTRA_CFLAGS makes life easier, since its
> required, but not part of the standard Fedora CFLAGS.

What about 'CFLAGS += ...' in the Makefile and a configure option --cflags,
would that work for you?

> - some of the added $(*DIR) bits are for convenience more than anything, but are
> fairly standard.

One thing that still bothers be is my use of LIBDIR. I know it's non-standard,
but I find it much more convenient when cross-compiling, because I only have to
set LIBDIR='lib' or 'lib32' and not LIBDIR='$PREFIX/lib'. I also don't really
see a use in installing the libs, apps and headers each into a different prefix.
I'm hard to convince, I admit, but my decisions are rarely set in stone, so I'm
sure if you give me strong, good reasons I may change my use of LIBDIR ;)

> - you've got lots of extraneous slashes -- $(DESTDIR)/$(PREFIX) works out to
> /somewhere//usr -- so I nuke all those.

Yeah, I noticed that, too, but i don't care because it's just a cosmetic change,
or isn't it?
Comment 15 Mamoru TASAKA 2007-02-08 13:06:54 EST
Just a question:

Should I wait this review until a new tarball is released?
Comment 16 Jarod Wilson 2007-02-08 16:49:57 EST
(In reply to comment #14)
> will do ... I'll drop you an email as soon as a new tarball is available.

Excellent, thank you.

> > For Fedora packages, libtool archives and static libs are forbidden, so it was
> > solely for the benefit of the package. I'll use a patch that doesn't nuke that,
> > and will instead remove the file from within the spec.
> 
> I would actually love to drop libtool, but it's a nice tool that adds '-fPIC'
> when needed (and other arch-dependent flags). But other than that, I find it
> horrible (what are these libtool archives for anyway? The compiler doesn't need
> them when linking against the library, right?).

I must admit to knowing very little about libtool or ways to replace it...

> > Other notes on the patch I'm using:
> > - splitting CFLAGS and EXTRA_CFLAGS makes packaging much easier. Fedora has a
> > standard set of CFLAGS that are supposed to be used on all packages, so
> > splitting off the -std=c99 bit into EXTRA_CFLAGS makes life easier, since its
> > required, but not part of the standard Fedora CFLAGS.
> 
> What about 'CFLAGS += ...' in the Makefile and a configure option --cflags,
> would that work for you?

I think a CFLAGS += might work, but the preferred way to get our CFLAGS into a
build is via an export, preferrably the one done by the %configure macro (which
also passes in libdir, bindir, sbindir, etc. info, which is part of why I was
dropping the additional *DIR bits into the Makefile).

> > - some of the added $(*DIR) bits are for convenience more than anything, but are
> > fairly standard.
> 
> One thing that still bothers be is my use of LIBDIR. I know it's non-standard,
> but I find it much more convenient when cross-compiling, because I only have to
> set LIBDIR='lib' or 'lib32' and not LIBDIR='$PREFIX/lib'. I also don't really
> see a use in installing the libs, apps and headers each into a different prefix.

Nah, we'd never install those bits in different prefixes either. This again goes
back to the %configure macro, which passes in a full path for LIBDIR. We never
touch LIB though, so I added that to add support for doing essentially what
you're doing w/LIBDIR at the moment. I could be missing something, but I though
that change would still allow you to build and install as you have been by
passing LIB='lib' or 'lib32'.

> I'm hard to convince, I admit, but my decisions are rarely set in stone, so I'm
> sure if you give me strong, good reasons I may change my use of LIBDIR ;)

Hope the above helped... :)

> > - you've got lots of extraneous slashes -- $(DESTDIR)/$(PREFIX) works out to
> > /somewhere//usr -- so I nuke all those.
> 
> Yeah, I noticed that, too, but i don't care because it's just a cosmetic change,
> or isn't it?

Mostly cosmetic, but also the right thing to do. :)


(In reply to comment #15)
> Just a question:
> 
> Should I wait this review until a new tarball is released?

Might as well.
Comment 17 Mamoru TASAKA 2007-03-03 05:51:06 EST
Any progress?
It seems that version 168 is released.
Comment 18 tomc 2007-03-03 06:44:46 EST
(In reply to comment #17)
> Any progress?
> It seems that version 168 is released.

No noteworthy changes.. I just implemented the RGB->YCbCr conversion as defined
by ITU-R BT.709 (before it was BT.601). Only the conversion matrix changed, a
normal user shouldn't notice any changes in the picture quality.
Comment 19 Jarod Wilson 2007-03-29 13:31:52 EDT
I see quite a few new seom tarballs (1.0-180 being the latest). Is there reason
to believe the latest is ready for me to revisit packaging?
Comment 20 tomc 2007-03-29 13:44:25 EDT
(In reply to comment #19)
> I see quite a few new seom tarballs (1.0-180 being the latest). Is there reason
> to believe the latest is ready for me to revisit packaging?

There were a few optimizations, though changeset 172 was quite important 
(http://neopsis.com/projects/seom/changeset/172). I'm currently rewriting the
file format and looking how to do sound capturing. All that work is done in
branches and once it is stable, I'll merge it with trunk.

If your last version is <172, I'd suggest to upgrade to at least 172. And you
may want to know that I've made quite big changes to the Makefile in 176, you'll
probably have to update your patch.
Comment 21 Mamoru TASAKA 2007-04-05 03:59:25 EDT
( I certainly remember I am reviewing this bug. When
  new package is ready, please upload and I will check it )
Comment 22 Mamoru TASAKA 2007-05-28 09:50:01 EDT
Any news on this package?
Comment 23 tomc 2007-05-28 10:03:25 EDT
179 is the latest version of trunk (main development branch). It works
reasonably well and I need no need for an update. It will take some more time to
merge the branch with the trunk, don't count on that it will happen anytime soon.
Comment 24 Mamoru TASAKA 2007-06-04 23:36:12 EDT
Jarod, would you try to repackage, again?
Comment 25 Mamoru TASAKA 2007-06-28 13:40:19 EDT
Please let me know how we should treat this review request.
Comment 26 Jarod Wilson 2007-06-28 14:11:14 EDT
Blah, haven't had time to touch beryl much of late. With the pending release of
something from the compiz-fusion (compiz/beryl merger), I'm inclined to just
deep-six this for now. Unless of course part of compiz-fusion needs seom as well...
Comment 27 tomc 2007-06-28 14:18:59 EDT
Yukon (http://neopsis.com/projects/yukon/) needs seom, it would be nice to
create a package for that as well. Do you want me to create a new bug for that?
AFAIK nothing from compiz-fusion needs seom, unless someone writes a vidcap
plugin based on seom.
Comment 28 Mamoru TASAKA 2007-06-28 14:26:40 EDT
(In reply to comment #27)
> Yukon (http://neopsis.com/projects/yukon/) needs seom, it would be nice to
> create a package for that as well. Do you want me to create a new bug for that?

"For that" means:
* you want to take over seom maintainership?
* or you want to create a review request for yukon?
Comment 29 Jarod Wilson 2007-06-28 14:30:34 EDT
3rd option:
* or both?

:)

I'd be happy to give up this package though. My time available for anything
outside kernel-space (aka "my day job") is rather limited these days...
Comment 30 tomc 2007-06-28 14:34:29 EDT
(In reply to comment #28)
> "For that" means:
> * you want to take over seom maintainership?
> * or you want to create a review request for yukon?
> 

I'm not running fedora. But it would be nice if fedora users could install yukon
from an official package. Do you want me to create a new bug to track the review
request for a yukon package?
Comment 31 Jarod Wilson 2007-06-28 14:45:01 EDT
Generally, the person who is going to maintain the package for fedora opens the
review request. I think there's a package wish list in the fedora wiki, but
yeah, someone has to actually step up to maintain the package before it can be
reviewed for inclusion.
Comment 32 Mamoru TASAKA 2007-06-28 15:05:22 EDT
Tomc, if you want to be a yukon maintainer on Fedora, you have
to open a new review request (with creating the spec/srpm of yukon).

And IMO you may also want to be a maintainer for seom on Fedora.
Comment 33 Mamoru TASAKA 2007-09-27 08:47:59 EDT
I think we should once close this bug. Any comments?
Comment 34 Jarod Wilson 2007-09-27 09:06:51 EDT
Agreed. If someone else wants to pick up the ball and run with it, they're more
than welcome, but for now, closing bug WONTFIX.
Comment 35 Mamoru TASAKA 2007-09-27 09:22:04 EDT
Thank you.

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