After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 733602 - Add lame encoder to cerbero
Add lame encoder to cerbero
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: packages
unspecified
Other Windows
: Normal enhancement
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-07-23 13:33 UTC by Lubosz Sarnecki
Modified: 2014-09-26 07:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch that adds lame encoder (2.94 KB, patch)
2014-07-23 13:34 UTC, Lubosz Sarnecki
needs-work Details | Review
Add lame patch (3.46 KB, patch)
2014-07-23 14:27 UTC, Lubosz Sarnecki
needs-work Details | Review
lame and nasm (7.32 KB, patch)
2014-07-28 13:01 UTC, Lubosz Sarnecki
needs-work Details | Review
Add lame (6.04 KB, patch)
2014-09-24 11:03 UTC, Lubosz Sarnecki
none Details | Review
Add lame encoder. Without optimizations. (4.29 KB, patch)
2014-09-25 12:24 UTC, Lubosz Sarnecki
committed Details | Review

Description Lubosz Sarnecki 2014-07-23 13:33:47 UTC
Having a mp3 encoder in cerbero would be cool.

Tested on windows.
Comment 1 Lubosz Sarnecki 2014-07-23 13:34:20 UTC
Created attachment 281475 [details] [review]
Patch that adds lame encoder
Comment 2 Sebastian Dröge (slomo) 2014-07-23 13:58:37 UTC
Review of attachment 281475 [details] [review]:

::: recipes/gst-plugins-ugly-1.0-static.recipe
@@ +14,2 @@
     files_plugins_codecs_restricted_devel = [
              'libgsta52dec',

You also need to add the plugin here

::: recipes/gst-plugins-ugly-1.0.recipe
@@ +14,2 @@
     files_plugins_codecs_restricted = [
              'lib/gstreamer-1.0/libgsta52dec%(mext)s',

and here

::: recipes/lame.recipe
@@ +5,3 @@
+    stype = SourceType.TARBALL
+    configure_options = ' --enable-static'
+    url = 'http://switch.dl.sourceforge.net/project/lame/lame/3.99/lame-3.99.5.tar.gz'

And lame:libs has to go into the relevant package


Is this already building lame with the assembler optimizations?
Comment 3 Lubosz Sarnecki 2014-07-23 14:27:09 UTC
Created attachment 281481 [details] [review]
Add lame patch

I added the --enable-nasm option, taken from the Arch Linux build:

https://projects.archlinux.org/svntogit/packages.git/tree/trunk/PKGBUILD?h=packages/lame

I added libgstlame to the library list.

Also used unix line endings and UTF8 this time ;)
Comment 4 Sebastian Dröge (slomo) 2014-07-24 11:03:33 UTC
Comment on attachment 281481 [details] [review]
Add lame patch

You need to add lame:libs to the gstreamer-1.0-codecs-restricted.package still

Also --enable-nasm requires nasm, which we don't depend on right now... right? Is there also a mode that can work with yasm?
Comment 5 Lubosz Sarnecki 2014-07-24 14:25:26 UTC
I cannot find a configure flag for yasm. I guess a nasm recipe would be required for the assembly build. Should I add it?
Comment 6 Sebastian Dröge (slomo) 2014-07-24 14:48:17 UTC
Yes, but it would be a build-tool and part of bootstrap
Comment 7 Lubosz Sarnecki 2014-07-28 13:01:05 UTC
Created attachment 281866 [details] [review]
lame and nasm

added nasm to linux distros bootstrap. added nasm recipe.
Comment 8 Sebastian Dröge (slomo) 2014-08-03 09:08:41 UTC
Review of attachment 281866 [details] [review]:

As you said that yasm supports nasm syntax, let's just not worry about yet another build tool :) Just needs checking if it actually uses yasm during the build... does it?

Otherwise looks good
Comment 9 Sebastian Dröge (slomo) 2014-09-04 11:22:09 UTC
Ping?
Comment 10 Lubosz Sarnecki 2014-09-11 09:53:43 UTC
This pops up in the configure step of lame with --enable-nasm on Windows, when only yasm is installed.

checking for nasm... no

I guess the optimizations are not used, but it builds fine.
here is the build log https://gist.github.com/lubosz/a7a3cc834b5f729d63a4

So I guess if you don't want to ship nasm and have no optimizations on Windows, then take the first patch. Otherwise the second one.

When cerbero would be ported to msys2 then shipping nasm will not be required, since its packaged there in all flavors.


bmonkey@bcarbon:~$ pacman -Ss nasm
mingw32/mingw-w64-i686-nasm 2.11.02-1
    An 80x86 assembler designed for portability and modularity (mingw-w64)
mingw32/mingw-w64-i686-yasm 1.2.0-2
    A rewrite of NASM to allow for multiple syntax supported (NASM, TASM, GAS, etc.) (mingw-w64)
mingw64/mingw-w64-x86_64-nasm 2.11.02-1
    An 80x86 assembler designed for portability and modularity (mingw-w64)
mingw64/mingw-w64-x86_64-yasm 1.2.0-2
    A rewrite of NASM to allow for multiple syntax supported (NASM, TASM, GAS, etc.) (mingw-w64)
msys/nasm 2.11.02-1 (base-devel) [installed]
    An 80x86 assembler designed for portability and modularity
msys/yasm 1.3.0-1 (base-devel) [installed]
    A rewrite of NASM to allow for multiple syntax supported (NASM, TASM, GAS, etc.)
msys/yasm-devel 1.3.0-1 (base-devel) [installed]
    A rewrite of NASM to allow for multiple syntax supported (NASM, TASM, GAS, etc.)
Comment 11 Edward Hervey 2014-09-11 12:58:12 UTC
afaiu, yasm can be used as-is to replace nasm. The only thing missing seems to be how lame's configure is looking for it.
Comment 12 Sebastian Dröge (slomo) 2014-09-12 10:52:31 UTC
Lubosz, can you change the patch accordingly?
Comment 13 Lubosz Sarnecki 2014-09-24 11:03:48 UTC
Created attachment 286969 [details] [review]
Add lame

Just adding a lame recipe.
Add lame recipe to ugly plugins and set build flags for lame.
Enabling nasm in lame recipe.
Adding nasm only to distro package manager deps, not as a recipe.
lame builds fine without nasm, even when the nasm flag is set.
Comment 14 Lubosz Sarnecki 2014-09-24 18:17:16 UTC
Using yasm is pretty easy, is can be achieved changing one line in configure.in
https://github.com/lubosz/lame/commit/782172def34e19da7323795d2779295387c0e9cb

Although autoconf needs to be run, to generate configure.

The assembler files are located here:
https://github.com/lubosz/lame/tree/yasm/libmp3lame/i386

As the folder suggests, the build allows only optimizations for i386 architectures.

With this little hack, I was able to test the build with -elf64
https://github.com/lubosz/lame/commit/55c4e706e7de58678d251b73d295552925115d22

But the linker fails:
/usr/bin/ld: ../libmp3lame/i386/.libs/liblameasmroutines.a(choose_table.o): relocation R_X86_64_32 against `choose_table_MMX' can not be used when making a shared object; recompile with -fPIC

So I guess, with a patch like 782172def34e19da7323795d2779295387c0e9cb, yasm could be possible. But it's supported for i386 only anyway.
Comment 15 Sebastian Dröge (slomo) 2014-09-25 08:03:40 UTC
So let's just forget about the assembly optimizations, who cares about 32 bit x86 :)

Which patch should be merged now?
Comment 16 Lubosz Sarnecki 2014-09-25 12:24:22 UTC
Created attachment 287068 [details] [review]
Add lame encoder. Without optimizations.

Current version, without optimizations.
Comment 17 Sebastian Dröge (slomo) 2014-09-26 07:59:15 UTC
commit 90b66d2bbd48dfd138d66d205ca95eb4d5422c1f
Author: Lubosz Sarnecki <lubosz@gmail.com>
Date:   Thu Sep 25 14:19:57 2014 +0200

    recipes: Add lame
    
    https://bugzilla.gnome.org/show_bug.cgi?id=733602