[Bug 118] Review Request: cinelerra-cv - Advanced audio and video capturing, compositing, and editing

[RPM Fusion Bugzilla]

http://bugzilla.rpmfusion.org/show_bug.cgi?id=118


Hans de Goede <j.w.r.degoede at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |3
             Status|NEW                         |ASSIGNED




--- Comment #29 from Hans de Goede <j.w.r.degoede at gmail.com>  2010-12-24 14:27:47 ---
(In reply to comment #28)
> Still interested in doing this package - feel free to do the review :)

Good!

I just took a quick look, I notice that you use a git snapshot, but you don't
provide instructions on how to generate the tarbal, making it impossible to
verify if the sources in the SRPM match upstream, which is a must.

I also noticed that CinelerraCV 2.1.5 is out (and from a later date then your
git snapshot), so perhaps you can upgrade to that rather then using a git
snapshot? That will also fix my verify problem.

Other then that from a quick glance everything looks good.

Some remarks about the shmmax tweaking though:

This:
  if [ $(grep kernel.shmmax %{_sysconfdir}/sysctl.conf |wc -l) == 0 ] ; then
Can be done simpler as this:
  if ! grep -q kernel.shmmax %{_sysconfdir}/sysctl.conf; then

I also wonder if differentiating between the runtime and boottime config with
the 2 checks is a good idea. If the user has already set a shmmax in his
sysctl.conf, I agree we should not overwrite it, but if it is too low, this
means cinelerra will work directly after install, but not after the first boot.

Would it not be better to remove this:
  if [ $(/sbin/sysctl -n -q kernel.shmmax) -lt 2147483647 ] ; then
    # Hack for the package to work "out of the box".
    /sbin/sysctl -q -w kernel.shmmax=0x7fffffff
  fi

And change the second if block to:

  if ! grep -q kernel.shmmax %{_sysconfdir}/sysctl.conf; then
    # Hack for the package to work "out of the box".
    /sbin/sysctl -q -w kernel.shmmax=0x7fffffff
    # Hack for the package to work on reboot.
    echo -e "\n# Added by Cinelerra RPM from RPM Fusion\nkernel.shmmax =
0x7fffffff" >> %{_sysconfdir}/sysctl.conf
  fi

This way either shmmax is left completely unchanged, or it is set to the same
value directly after install as it will be after a reboot.

I guess this should be combined with a small sh wrapper script for cinelerra-cv
which does the "if [ $(/sbin/sysctl -n -q kernel.shmmax) -lt 2147483647 ] ;
then" check, and if there is not enough shm, then prints an error using
zenity --error --text="..." and exits, or does cinelerra already checks for
this
itself ?

I hope to see a package updated to 2.1.5 from you soon, and then I'll do a full
review.

Regards,

Hans


-- 
Configure bugmail: http://bugzilla.rpmfusion.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.