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 584890 - AMR plugins based on Opencore codecs
AMR plugins based on Opencore codecs
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-ugly
git master
Other Linux
: Normal enhancement
: 0.10.13
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 551639 582150 (view as bug list)
Depends on: 584897
Blocks:
 
 
Reported: 2009-06-05 06:00 UTC by Iago Toral
Modified: 2009-07-27 18:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
AMR plugins based on Opencore codecs (890.96 KB, application/x-compressed-tar)
2009-06-05 06:14 UTC, Iago Toral
  Details
Opencore AMR plugins, patch for -bad (11.83 KB, patch)
2009-07-15 05:23 UTC, Iago Toral
none Details | Review
Opencore AMR plugins, patch for -ugly (535.47 KB, patch)
2009-07-15 05:24 UTC, Iago Toral
none Details | Review
Opencore AMR plugins, patch for -bad (9.17 KB, patch)
2009-07-16 06:05 UTC, Iago Toral
none Details | Review
Opencore AMR plugins, patch for -ugly (526.76 KB, patch)
2009-07-16 06:07 UTC, Iago Toral
none Details | Review
Opencore AMR plugins, patch for -bad (14.83 KB, application/x-gzip)
2009-07-22 16:05 UTC, Iago Toral
  Details
Opencore AMR plugins, patch for -ugly (11.36 KB, application/x-gzip)
2009-07-22 16:06 UTC, Iago Toral
  Details
Opencore AMR plugins, patch for -bad (78.39 KB, patch)
2009-07-24 14:59 UTC, Iago Toral
committed Details | Review
Opencore AMR plugins, patch for -ugly (53.15 KB, patch)
2009-07-24 15:00 UTC, Iago Toral
committed Details | Review

Description Iago Toral 2009-06-05 06:00:09 UTC
There current AMR plugins in -bad and -ugly require you to download a tarball from the standards website and unzip it into the plugins structure. There is no need for this anymore as there is Apache licensed implementation of encoders and decoders for both AMR WB and AMR NB in Android.

Opencore provides implementation for the AMR-NB decoder and encoder and for the AMR-WB decoder (no encoder).

This task was proposed by Christian Schaller:
http://sourceforge.net/mailarchive/message.php?msg_name=1242057217.28145.140.camel%40localhost.localdomain

The Opencore AMR codecs have been ripped out of Android already by the ffmpeg team:
http://gitorious.org/opencore-amr/opencore-amr

I took that code and the current gstreamer amr plugins in -bad and -ugly and joined them together. I also added support for the preset inteface to the AMR-NB encoder.

I will attach the implementation soon.
Comment 1 Iago Toral 2009-06-05 06:14:30 UTC
Created attachment 135998 [details]
AMR plugins  based on Opencore codecs

This is an implementation of the gstreamer AMR plugins based on the OpenCore codecs to start discussion on how to merge this upstream. It provides these elements:

amrnbdec: AMR-NB decoder
amrnbenc: AMR-NB encoder
amrwbdec: AMR-WB decoder

The gstreamer codec plugins are the same hosted already in -bad and -ugly with very minor modifications and including support for the preset interface in the case of the encoder, the Opencore AMR codecs have been ripped and included as part of the plugins.

The Opencore code is licensed under the  Apache 2 license, while the gstreamer plugins stay LGPL.
Comment 2 Christian Fredrik Kalager Schaller 2009-06-05 08:16:00 UTC
*** Bug 582150 has been marked as a duplicate of this bug. ***
Comment 3 Stefan Sauer (gstreamer, gtkdoc dev) 2009-06-07 21:01:27 UTC
Cool. One comment, the preset iface is only useful if tehre are settings (aka gobject properties).
Comment 4 Iago Toral 2009-06-08 07:54:04 UTC
(In reply to comment #3)
> Cool. One comment, the preset iface is only useful if tehre are settings (aka
> gobject properties).

Yes, in this case there is the band-mode property for the amr-nb encoder.
Comment 5 Christian Fredrik Kalager Schaller 2009-06-09 09:48:38 UTC
Hi Iago,
Just so you know what the plan is. Once the current freeze is over the plan is to commit these two directly to -ugly and replace both of the two previous modules in Ugly and bad.

Comment 6 Iago Toral 2009-06-09 14:10:17 UTC
Great! thanks for the info Christian.
Comment 7 Bastien Nocera 2009-06-12 17:55:22 UTC
*** Bug 551639 has been marked as a duplicate of this bug. ***
Comment 8 Christian Fredrik Kalager Schaller 2009-06-29 13:54:10 UTC
Iago, any chance you could turn this into a patch against gst-plugins-ugly? 
Comment 9 Iago Toral 2009-06-30 06:59:49 UTC
Yes, no problem. I'll do that and attach the patch here.
Comment 10 Sebastian Dröge (slomo) 2009-07-08 16:40:22 UTC
Any news on this?
Comment 11 Iago Toral 2009-07-13 07:20:20 UTC
(In reply to comment #10)
> Any news on this?

Sorry for being late, I was quite busy these weeks and could not work on this. I'll upload the patch during this week.
Comment 12 Iago Toral 2009-07-15 05:23:20 UTC
Created attachment 138425 [details] [review]
Opencore AMR plugins, patch for -bad

Patch for -bad
Comment 13 Iago Toral 2009-07-15 05:24:06 UTC
Created attachment 138426 [details] [review]
Opencore AMR plugins, patch for -ugly

Patch for -ugly
Comment 14 Iago Toral 2009-07-15 05:35:29 UTC
Few comments on the patches to ease the review work:

- Removed amrnb plugin from -ugly (ext), which contained:
  amrnbenc
  amrnbdec
  amrnparse
- Removed armwb pluign from -bad (ext), which contained:
  amrwbenc
  amrwbdec
  amrwparse
- Added amr plugin in -ugly (gst), which contains:
  amrnbenc
  amrnbdec
  amrnbparse
  amrwbdec,
  amrwbparse
  (*) No amrwbenc, since opencore does not implement the encoder.
  (*) This plugin contains the opencore amr codecs, this is why I put it in gst and not in ext.
  (*) I joined together the amr-nb and amr-wb stuff in a single amr plugin because they are both based on the same code from opencore, but maybe you prefer having both separated...
- Moved documentation related to amr-nb and amr-wb from -bad to -ugly (and adapted it appropriately)
- Adapted unit tests

Hopefully I did not miss anything :). If you find something wrong let me know and I'll fix it.

Also, while preparing the patch I found an amrparse plugin in -bad (gst). At first glance it looks like this plugin implements the same stuff as the amrnbparse and amrwbparse elements, I do not know if there is a good reason to have these three elements or if it is just duplicated code.
Comment 15 Stefan Sauer (gstreamer, gtkdoc dev) 2009-07-15 11:32:34 UTC
We should remove the parsers from arm plugins. The idea is to have one big parser plugin in good with all the parsers. Also the parsers are usualy not affected by the codec license situation.

Iago, could you remove the parsers from the patch against -ugly.
Comment 16 Iago Toral 2009-07-15 11:50:35 UTC
Yes, makes sense. Actually, I thought on doing that already when I found about this, but in the end decided to wait just in case I was missing something.

I'll provide a new version of the patch for -ugly removing those parsers later today or tomorrow.
Comment 17 Tim-Philipp Müller 2009-07-15 12:02:22 UTC
I think we should keep parts of the amrwb plugin in -bad for the time being, so that there still is an amrwb encoder available in GStreamer. So maybe just remove amrwbdec and amrwparse from the amrwb plugin in -bad, but keep amrwbenc until wideband encoding is added to the opencore codecs? 

Comment 18 Iago Toral 2009-07-15 18:33:13 UTC
I just found this (from Debian Sid):

$ apt-cache search opencore

libopencore-amrnb-dev - Adaptive Multi Rate speech codec - development files
libopencore-amrnb0 - Adaptive Multi Rate speech codec - shared library
libopencore-amrwb-dev - Adaptive Multi-Rate - Wideband speech codec - development files
libopencore-amrwb0 - Adaptive Multi-Rate - Wideband speech codec - shared library

which provide the opencore amr codecs already, it is the same stuff I included in my patch as part of the amr plugins (I included the codec sources as part of the plugin), guess it does not make sense now... should I rewrite the patch to use these packages instead and keep it as a plugin with external dependencies?
Comment 19 Tim-Philipp Müller 2009-07-15 19:02:25 UTC
> $ apt-cache search opencore 
> libopencore-amrnb-dev - Adaptive Multi Rate speech codec - development files
> ...

These are not in debian proper though but 'only' in the external debian-multimedia repo, as I understand it (not that that really matters much).


> which provide the opencore amr codecs already, it is the same stuff I included
> in my patch as part of the amr plugins (I included the codec sources as part of
> the plugin), guess it does not make sense now... should I rewrite the patch to
> use these packages instead and keep it as a plugin with external dependencies?

I would be in favour of that, since it just seems The Right Thing To Do. However, it would probably be inconvenient for users of other distros where no such packages exist yet. I think, however, that we should nevertheless just forge ahead with this; livna etc. will likely package it soon enough as well then. In the mean time, Christian could create source rpms for people :D

Let's see what others think... 
Comment 20 Bastien Nocera 2009-07-15 19:32:38 UTC
Happy either way, as long as there's actually an upstream tracker/code repo to track necessary changes. Are we also sure that they don't break ABI all the time, and that the libraries are from upstream (not a packager's work)?

The git repo mentioned in comment 0 doesn't exist anymore.
Comment 21 Christian Fredrik Kalager Schaller 2009-07-15 19:38:46 UTC
Those debian packages where briefly discussed during GUADEC and my opinion then as now is that we should only switch to using them if there seems to be a serious upstream available. Meaning that there is actually someone maintaining that code as a separate project, who  makes sure to pull updates from Android, and who makes releases the different distros can package. Without even a real website I am a little concerned that at this point these libraries are not really maintained and thus is would be better to keep our own code copy for now.
Comment 23 Iago Toral 2009-07-16 06:05:50 UTC
Created attachment 138499 [details] [review]
Opencore AMR plugins, patch for -bad

Updated version of the patch for -bad that keeps the original amr-wb encoder from ext/amrwb.
Comment 24 Iago Toral 2009-07-16 06:07:25 UTC
Created attachment 138500 [details] [review]
Opencore AMR plugins, patch for -ugly

Updated version of the patch for -ugly that removes redundant amrnbparse and amrwbparse elements.
Comment 25 Iago Toral 2009-07-16 06:16:12 UTC
Ok, while people reach consensus about these opencore amr packages I just uploaded an updated version of the patches that implements the changes suggested:
  - remove amrnbparse
  - remove amrwbparse
  - keep amrwbenc from -bad

According to the link LRN posted above it looks like the project is still alive but has been moved to sourceforge, I'll send an email to the developers and see if they can clarify some of the questions people dropped here.
Comment 26 Tim-Philipp Müller 2009-07-16 09:24:30 UTC
> I'll send an email to the developers and see if they can
> clarify some of the questions people dropped here.

FWIW, I sent them an email yesterday as well :)


Comment 27 Iago Toral 2009-07-20 05:48:14 UTC
So, after talking to the libopencore-amr developers, it looks like:

- They intend to maintain the library in the future.
- They do not intend to create packages for other distros (although they
would accept patches for that). 
   - We could ask distros to create the packages if the developers create regular releases.
- They do not expect to break API/ABI.
Comment 28 Christian Fredrik Kalager Schaller 2009-07-22 10:01:03 UTC
So I guess the consensus is that with these plans in place there is no reason for us not to depend on this external library instead of keeping our own copy. So feel free to go ahead and update the plugins to use these libraries. Maybe add a readme to the plugin directory pointing people to the place to get the tarball releases of the library.
Comment 29 Iago Toral 2009-07-22 16:05:22 UTC
Created attachment 139004 [details]
Opencore AMR plugins, patch for -bad
Comment 30 Iago Toral 2009-07-22 16:06:06 UTC
Created attachment 139005 [details]
Opencore AMR plugins, patch for -ugly
Comment 31 Iago Toral 2009-07-22 16:24:01 UTC
Attached patches that use external opencore-amr libraries from http://sourceforge.net/projects/opencore-amr/ instead of copying the opencore amr codec source code.

After applying the patch, this is the result:

gst-plugins-bad/ext/amrwbenc
  - Contains the amrwbenc plugin that provides element amrwbenc based on the reference implementation from 3gpp.

gst-plugins-ugly/ext/amrwbdec
  - Contains the amrwbdec plugin that provides element amrwbdec (based on opencore codec)

gst-plugins-ugly/ext/amrnb
  - amrwnb plugin that provides elements:
    - amrnbdec, based on opencore codec.
    - amrnbenc, based on opencore codec.

This patch also removes the element amrnbparse and amrwbparse, amrparse from -bad should be used instead.

Added README files as Christian suggested pointing to the the opencore-amr site for others to obtain the source code of the opencore amr codec.

Edited the docs in -bad and -ugly according to these changes.
Comment 32 Michael Monreal 2009-07-22 21:48:16 UTC
Is opencore-amr a "problematic" package, e.g. will it be packaged for Fedora?
Comment 33 Christian Fredrik Kalager Schaller 2009-07-22 22:09:08 UTC
It is, Nokia and others are collecting AMR patent royalities, so it will go into -ugly and not good. Thus it will be packaged by Livna/rpmfusion or similar sites instead of by Fedora themselves.
Comment 34 Iago Toral 2009-07-23 06:17:12 UTC
For reviewers: the patches are gzip files... 

bugzilla insists in keeping mime-type text/plain even when I set it to application/x-gzip
Comment 35 Iago Toral 2009-07-23 07:55:40 UTC
Mmm.. I just realized this patch does not include the preset interface for the amrnb encoder. I'll provide a new patch that includes that (and I guess I can do the same for the amrwb encoder in -bad).
Comment 36 André Klapper 2009-07-23 09:45:20 UTC
Comment on attachment 139005 [details]
Opencore AMR plugins, patch for -ugly

Wrong attachment mime type was because "patch" flag was set.
Comment 37 Iago Toral 2009-07-24 14:59:04 UTC
Created attachment 139161 [details] [review]
Opencore AMR plugins, patch for -bad
Comment 38 Iago Toral 2009-07-24 15:00:22 UTC
Created attachment 139162 [details] [review]
Opencore AMR plugins, patch for -ugly
Comment 39 Iago Toral 2009-07-24 15:03:31 UTC
Attached new patches providing support for presets interface in both encoders. The presets I include in the .prs files are just examples, probably somebody else should define presets that make more sense.
Comment 40 Sebastian Dröge (slomo) 2009-07-27 17:43:27 UTC
Is it planned to add a amrwb encoder to the opencore library?
Whatever, I'll take a look at those patches :)
Comment 41 Sebastian Dröge (slomo) 2009-07-27 18:14:35 UTC
commit d6bb14dade29ed36e4803e0ea4efff665da68dd3
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Mon Jul 27 19:59:32 2009 +0200

    amrwbenc: Fix compilation

commit 29e39080324a4a5a3b4d5bbc3fc79213090c6b0a
Author: Iago Toral <itoral@igalia.com>
Date:   Mon Jul 27 19:55:27 2009 +0200

    amrwb: Remove AMR-WB parser and decoder and rename encoder plugin from amrwb
    
    Partially fixes bug #584890.

commit f01429ae16424e5417f89e0ecb2205dc38c1e77f
Author: Iago Toral <itoral@igalia.com>
Date:   Mon Jul 27 20:12:20 2009 +0200

    amr: Add AMR-WB decoder and AMR-NB encoder and decoder
    
    These are based on the OpenCore codecs.
    
    Fixes bug #584890.