diff mbox series

[bug#40677,v4] gnu: Add ffmpeg-jami.

Message ID 20200513181234.27548-1-tona_kosmicznego_smiecia@interia.pl
State Accepted
Headers show
Series [bug#40677,v4] gnu: Add ffmpeg-jami. | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job

Commit Message

Jan Wielkiewicz May 13, 2020, 6:12 p.m. UTC 
This package is needed, because Jami uses a modified version
of ffmpeg, which provides GPU hardware acceleration, automatical
adapting of bitrate and extra codecs. Because of the configure flags
list being long, it is better to keep them separated in variables,
instead of littering the package definition.

* gnu/packages/jami.scm (ffmpeg-jami, %ffmpeg-default-configure-flags,
%ffmpeg-linux-configure-flags, %ffmpeg-linux-x86-configure-flags):
New variables.
(ffmpeg-compose-configure-flags): New procedure.
(libring)[inputs]: Use ffmpeg-jami instead of ffmpeg.
---
 gnu/packages/jami.scm | 255 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 253 insertions(+), 2 deletions(-)

Comments

Mathieu Othacehe May 14, 2020, 7:05 a.m. UTC | #1 
Hello Jan,

> +;; This procedure composes the configure flags list for ffmpeg-jami.
> +(define (ffmpeg-compose-configure-flags)
> +  (let* ((flags %ffmpeg-default-configure-flags)
> +         (append-flags (lambda (l)
> +                         (set! flags (append flags l))))

It's better to avoid using "set!".

> +         (system=? (lambda (s)
> +                     (string-prefix? %current-system s))))

This should be (%current-system), plus I think arguments should be
transposed.

> +    (if (string-contains %current-system "linux")
> +        (begin (append-flags %ffmpeg-linux-configure-flags)
> +               (cond ((or (system=? "i686")
> +                          (system=? "x86_64"))
> +                      (append-flags %ffmpeg-linux-x86-configure-flags))
> +                     ((system=? "x86_64")
> +                      (append-flags '("--arch=x86_64")))

If the first branch of the cond succeeds, we will never add this flag.

Plus, it seems than ffmpeg is able to detect the running system. So I
would suggest to do this:

--8<---------------cut here---------------start------------->8---
;; This procedure composes the configure flags list for ffmpeg-jami.
(define (ffmpeg-compose-configure-flags)
  (define (system=? s)
    (string-prefix? s (%current-system)))

  `(,@%ffmpeg-default-configure-flags
    ;; Add Linux specific flags.
    ,@(if (string-contains %current-system "linux")
          %ffmpeg-linux-configure-flags
          '())
    ,@(if (or (system=? "i686") (system=? "x86_64"))
          %ffmpeg-linux-x86-configure-flags
          '())))
--8<---------------cut here---------------end--------------->8---

What do you think?

Thanks,

Mathieu
Jan Wielkiewicz May 14, 2020, 12:43 p.m. UTC | #2 
On Thu, 14 May 2020 09:05:03 +0200
Mathieu Othacehe <othacehe@gnu.org> wrote:

> Hello Jan,
> 
> 
> It's better to avoid using "set!".
> 
What should I use instead?

> > +         (system=? (lambda (s)
> > +                     (string-prefix? %current-system s))))  
> 
> This should be (%current-system), plus I think arguments should be
> transposed.
Okay

> > +    (if (string-contains %current-system "linux")
> > +        (begin (append-flags %ffmpeg-linux-configure-flags)
> > +               (cond ((or (system=? "i686")
> > +                          (system=? "x86_64"))
> > +                      (append-flags
> > %ffmpeg-linux-x86-configure-flags))
> > +                     ((system=? "x86_64")
> > +                      (append-flags '("--arch=x86_64")))  
> 
> If the first branch of the cond succeeds, we will never add this flag.
That's what happens when you code late in night :)
I should sit down with a cup of coffee and read Guile's manual seriously
this time. Sorry for making your code review harder.

> Plus, it seems than ffmpeg is able to detect the running system. So I
> would suggest to do this:
> 
> --8<---------------cut here---------------start------------->8---
> ;; This procedure composes the configure flags list for ffmpeg-jami.
> (define (ffmpeg-compose-configure-flags)
>   (define (system=? s)
>     (string-prefix? s (%current-system)))
> 
>   `(,@%ffmpeg-default-configure-flags
>     ;; Add Linux specific flags.
>     ,@(if (string-contains %current-system "linux")
>           %ffmpeg-linux-configure-flags
>           '())
>     ,@(if (or (system=? "i686") (system=? "x86_64"))
>           %ffmpeg-linux-x86-configure-flags
>           '())))
> --8<---------------cut here---------------end--------------->8---
> 
> What do you think?

I think  %ffmpeg-linux-x86-configure-flags should be added only if
linux is present, not just when on i686 or x86_64. I called it
"%ffmpeg-linux-x86...", because it was inside of the "ifdef HAVE_LINUX"
condition. But there was also a comment saying "Desktop Linux", which
as always means very little. I'm not sure if it really requires the
Linux kernel there or what.

What about the output of the procedure? Is it okay for the list to be
not proper? It will look something like this:
((flag1 flag2 ... flagN) (flag1 flag2 ... flagN))
Is it okay because everything is treated as a pair in a recursive
manner?

Other than this, it will be good.


> Thanks,
> 
> Mathieu
Mathieu Othacehe May 15, 2020, 6:59 a.m. UTC | #3 
Hey,

> What should I use instead?

Here you can create the list statically using quasi-quotes.

>> If the first branch of the cond succeeds, we will never add this flag.
> That's what happens when you code late in night :)
> I should sit down with a cup of coffee and read Guile's manual seriously
> this time. Sorry for making your code review harder.

No worries.

>
> I think  %ffmpeg-linux-x86-configure-flags should be added only if
> linux is present, not just when on i686 or x86_64. I called it
> "%ffmpeg-linux-x86...", because it was inside of the "ifdef HAVE_LINUX"
> condition. But there was also a comment saying "Desktop Linux", which
> as always means very little. I'm not sure if it really requires the
> Linux kernel there or what.

Yes you are right.

> What about the output of the procedure? Is it okay for the list to be
> not proper? It will look something like this:
> ((flag1 flag2 ... flagN) (flag1 flag2 ... flagN))
> Is it okay because everything is treated as a pair in a recursive
> manner?

Nope the list should be flat, see:
https://www.gnu.org/software/guile/manual/html_node/Expression-Syntax.html.

Thanks,

Mathieu
diff mbox series

Patch

diff --git a/gnu/packages/jami.scm b/gnu/packages/jami.scm
index dda787b3cd..2cf2f3aa10 100644
--- a/gnu/packages/jami.scm
+++ b/gnu/packages/jami.scm
@@ -59,7 +59,8 @@ 
   #:use-module (guix download)
   #:use-module (guix git-download)
   #:use-module (guix packages)
-  #:use-module (guix utils))
+  #:use-module (guix utils)
+  #:use-module (srfi srfi-1))
 
 (define %jami-version "20200401.1.6f090de")
 
@@ -186,6 +187,256 @@ 
                 "selftest: pjlib-test pjlib-util-test pjmedia-test"))
              #t)))))))
 
+;; The following variables are configure flags used by ffmpeg-jami.
+;; They're from the ring-project/daemon/contrib/src/ffmpeg/rules.mak
+;; file. We try to keep it as close to the official Jami package as
+;; possible to provide all the codecs and extra features that are
+;; the effect of patching ffmpeg.
+;;
+;; Web view of the file:
+;;https://review.jami.net/plugins/gitiles/ring-daemon/+/refs/heads/master/contrib/src/ffmpeg/rules.mak
+(define %ffmpeg-default-configure-flags
+  '(;; disable everything
+    "--disable-everything"
+    "--enable-zlib"
+    "--enable-gpl"
+    "--enable-swscale"
+    "--enable-bsfs"
+    "--disable-filters"
+    "--disable-programs"
+    "--disable-postproc"
+    "--disable-protocols"
+    "--enable-protocol=crypto"
+    "--enable-protocol=file"
+    "--enable-protocol=rtp"
+    "--enable-protocol=srtp"
+    "--enable-protocol=tcp"
+    "--enable-protocol=udp"
+    "--enable-protocol=unix"
+    "--enable-protocol=pipe"
+    ;; enable muxers/demuxers
+    "--disable-demuxers"
+    "--disable-muxers"
+    "--enable-muxer=rtp"
+    "--enable-muxer=g722"
+    "--enable-muxer=h263"
+    "--enable-muxer=h264"
+    "--enable-muxer=hevc"
+    "--enable-muxer=webm"
+    "--enable-muxer=ogg"
+    "--enable-muxer=pcm_s16be"
+    "--enable-muxer=pcm_s16le"
+    "--enable-demuxer=rtp"
+    "--enable-demuxer=mjpeg"
+    "--enable-demuxer=mjpeg_2000"
+    "--enable-demuxer=mpegvideo"
+    "--enable-demuxer=gif"
+    "--enable-demuxer=image_jpeg_pipe"
+    "--enable-demuxer=image_png_pipe"
+    "--enable-demuxer=image_webp_pipe"
+    "--enable-demuxer=matroska"
+    "--enable-demuxer=m4v"
+    "--enable-demuxer=mp3"
+    "--enable-demuxer=ogg"
+    "--enable-demuxer=flac"
+    "--enable-demuxer=wav"
+    "--enable-demuxer=ac3"
+    "--enable-demuxer=g722"
+    "--enable-demuxer=pcm_mulaw"
+    "--enable-demuxer=pcm_alaw"
+    "--enable-demuxer=pcm_s16be"
+    "--enable-demuxer=pcm_s16le"
+    "--enable-demuxer=h263"
+    "--enable-demuxer=h264"
+    "--enable-demuxer=hevc"
+    ;; enable parsers
+    "--enable-parser=h263"
+    "--enable-parser=h264"
+    "--enable-parser=hevc"
+    "--enable-parser=mpeg4video"
+    "--enable-parser=vp8"
+    "--enable-parser=vp9"
+    "--enable-parser=opus"
+    ;; encoders/decoders
+    "--enable-encoder=adpcm_g722"
+    "--enable-decoder=adpcm_g722"
+    "--enable-encoder=rawvideo"
+    "--enable-decoder=rawvideo"
+    "--enable-encoder=libx264"
+    "--enable-decoder=h264"
+    "--enable-encoder=pcm_alaw"
+    "--enable-decoder=pcm_alaw"
+    "--enable-encoder=pcm_mulaw"
+    "--enable-decoder=pcm_mulaw"
+    "--enable-encoder=mpeg4"
+    "--enable-decoder=mpeg4"
+    "--enable-encoder=libvpx_vp8"
+    "--enable-decoder=vp8"
+    "--enable-decoder=vp9"
+    "--enable-encoder=h263"
+    "--enable-encoder=h263p"
+    "--enable-decoder=h263"
+    "--enable-encoder=mjpeg"
+    "--enable-decoder=mjpeg"
+    "--enable-decoder=mjpegb"
+    "--enable-libspeex"
+    "--enable-libopus"
+    "--enable-libvpx"
+    "--enable-libx264"
+    "--enable-encoder=libspeex"
+    "--enable-decoder=libspeex"
+    "--enable-encoder=libopus"
+    "--enable-decoder=libopus"
+    ;; decoders for ringtones and audio streaming
+    "--enable-decoder=flac"
+    "--enable-decoder=vorbis"
+    "--enable-decoder=aac"
+    "--enable-decoder=ac3"
+    "--enable-decoder=eac3"
+    "--enable-decoder=mp3"
+    "--enable-decoder=pcm_u24be"
+    "--enable-decoder=pcm_u24le"
+    "--enable-decoder=pcm_u32be"
+    "--enable-decoder=pcm_u32le"
+    "--enable-decoder=pcm_u8"
+    "--enable-decoder=pcm_f16le"
+    "--enable-decoder=pcm_f24le"
+    "--enable-decoder=pcm_f32be"
+    "--enable-decoder=pcm_f32le"
+    "--enable-decoder=pcm_f64be"
+    "--enable-decoder=pcm_f64le"
+    "--enable-decoder=pcm_s16be"
+    "--enable-decoder=pcm_s16be_planar"
+    "--enable-decoder=pcm_s16le"
+    "--enable-decoder=pcm_s16le_planar"
+    "--enable-decoder=pcm_s24be"
+    "--enable-decoder=pcm_s24le"
+    "--enable-decoder=pcm_s24le_planar"
+    "--enable-decoder=pcm_s32be"
+    "--enable-decoder=pcm_s32le"
+    "--enable-decoder=pcm_s32le_planar"
+    "--enable-decoder=pcm_s64be"
+    "--enable-decoder=pcm_s64le"
+    "--enable-decoder=pcm_s8"
+    "--enable-decoder=pcm_s8_planar"
+    "--enable-decoder=pcm_u16be"
+    "--enable-decoder=pcm_u16le"
+    ;; encoders/decoders for images
+    "--enable-encoder=gif"
+    "--enable-decoder=gif"
+    "--enable-encoder=jpegls"
+    "--enable-decoder=jpegls"
+    "--enable-encoder=ljpeg"
+    "--enable-decoder=jpeg2000"
+    "--enable-encoder=png"
+    "--enable-decoder=png"
+    "--enable-encoder=bmp"
+    "--enable-decoder=bmp"
+    "--enable-encoder=tiff"
+    "--enable-decoder=tiff"
+    ;; filters
+    "--enable-filter=scale"
+    "--enable-filter=overlay"
+    "--enable-filter=amix"
+    "--enable-filter=amerge"
+    "--enable-filter=aresample"
+    "--enable-filter=format"
+    "--enable-filter=aformat"
+    "--enable-filter=fps"
+    "--enable-filter=transpose"
+    "--enable-filter=pad"))
+
+(define %ffmpeg-linux-configure-flags
+  '("--enable-pic"
+    "--extra-cxxflags=-fPIC"
+    "--extra-cflags=-fPIC"
+    "--target-os=linux"
+    "--enable-indev=v4l2"
+    "--enable-indev=xcbgrab"
+    "--enable-vdpau"
+    "--enable-hwaccel=h264_vdpau"
+    "--enable-hwaccel=mpeg4_vdpau"
+    "--enable-vaapi"
+    "--enable-hwaccel=h264_vaapi"
+    "--enable-hwaccel=mpeg4_vaapi"
+    "--enable-hwaccel=h263_vaapi"
+    "--enable-hwaccel=vp8_vaapi"
+    "--enable-hwaccel=mjpeg_vaapi"
+    "--enable-hwaccel=hevc_vaapi"
+    "--enable-encoder=h264_vaapi"
+    "--enable-encoder=vp8_vaapi"
+    "--enable-encoder=mjpeg_vaapi"
+    "--enable-encoder=hevc_vaapi"))
+
+;; ffnvcodec is not supported on ARM then we enable it here for i386 and x86_64
+(define %ffmpeg-linux-x86-configure-flags
+  '("--arch=x86"
+    "--enable-cuvid"
+    "--enable-ffnvcodec"
+    "--enable-nvdec"
+    "--enable-nvenc"
+    "--enable-hwaccel=h264_nvdec"
+    "--enable-hwaccel=hevc_nvdec"
+    "--enable-hwaccel=vp8_nvdec"
+    "--enable-hwaccel=mjpeg_nvdec"
+    "--enable-encoder=h264_nvenc"
+    "--enable-encoder=hevc_nvenc"))
+
+;; This procedure composes the configure flags list for ffmpeg-jami.
+(define (ffmpeg-compose-configure-flags)
+  (let* ((flags %ffmpeg-default-configure-flags)
+         (append-flags (lambda (l)
+                         (set! flags (append flags l))))
+         (system=? (lambda (s)
+                     (string-prefix? %current-system s))))
+    (if (string-contains %current-system "linux")
+        (begin (append-flags %ffmpeg-linux-configure-flags)
+               (cond ((or (system=? "i686")
+                          (system=? "x86_64"))
+                      (append-flags %ffmpeg-linux-x86-configure-flags))
+                     ((system=? "x86_64")
+                      (append-flags '("--arch=x86_64")))
+                     ((system=? "aarch64")
+                      (append-flags '("--arch=aarch64")))
+                     ((system=? "armhf")
+                      (append-flags '("--arch=armhf"))))))
+    flags))
+
+(define-public ffmpeg-jami
+  (package
+    (inherit ffmpeg)
+    (name "ffmpeg-jami")
+    (native-inputs
+     `(("sfl-patches" ,(jami-source))
+       ("libiconv" ,libiconv)
+       ,@(package-native-inputs ffmpeg)))
+    (supported-systems '("x86_64-linux" "i686-linux"
+                         "aarch64-linux" "armhf-linux"))
+    (arguments
+     (append
+      '(#:tests? #f)
+      (substitute-keyword-arguments (package-arguments ffmpeg)
+        ((#:configure-flags '())
+         (ffmpeg-compose-configure-flags))
+        ((#:phases phases)
+         `(modify-phases ,phases
+            (add-after 'unpack 'make-git-checkout-writable
+              (lambda _
+                (for-each make-file-writable (find-files "."))
+                #t))
+            (add-after 'unpack 'apply-patches
+              (lambda* (#:key inputs #:allow-other-keys)
+                (let ((jami-apply-dependency-patches ,jami-apply-dependency-patches))
+                  ;; These patches come from:
+                  ;; "ring-project/daemon/contrib/src/ffmpeg/rules.mak".
+                  (jami-apply-dependency-patches #:inputs inputs
+                                                 #:dep-name "ffmpeg"
+                                                 #:patches
+                                                 '("remove-mjpeg-log"
+                                                   "change-RTCP-ratio"
+                                                   "rtp_ext_abs_send_time"))
+                  #t))))))))))
+
 (define-public libring
   (package
     (name "libring")
@@ -197,7 +448,7 @@ 
        ("boost" ,boost)
        ("dbus-c++" ,dbus-c++)
        ("eudev" ,eudev)
-       ("ffmpeg" ,ffmpeg)
+       ("ffmpeg" ,ffmpeg-jami)
        ("flac" ,flac)
        ("gmp" ,gmp)
        ("gsm" ,gsm)