GNOME Bugzilla – Bug 584890
AMR plugins based on Opencore codecs
Last modified: 2009-07-27 18:14:35 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.
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.
*** Bug 582150 has been marked as a duplicate of this bug. ***
Cool. One comment, the preset iface is only useful if tehre are settings (aka gobject properties).
(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.
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.
Great! thanks for the info Christian.
*** Bug 551639 has been marked as a duplicate of this bug. ***
Iago, any chance you could turn this into a patch against gst-plugins-ugly?
Yes, no problem. I'll do that and attach the patch here.
Any news on this?
(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.
Created attachment 138425 [details] [review] Opencore AMR plugins, patch for -bad Patch for -bad
Created attachment 138426 [details] [review] Opencore AMR plugins, patch for -ugly Patch for -ugly
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.
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.
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.
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?
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?
> $ 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...
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.
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.
http://sourceforge.net/projects/opencore-amr/develop
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.
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.
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.
> 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 :)
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.
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.
Created attachment 139004 [details] Opencore AMR plugins, patch for -bad
Created attachment 139005 [details] Opencore AMR plugins, patch for -ugly
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.
Is opencore-amr a "problematic" package, e.g. will it be packaged for Fedora?
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.
For reviewers: the patches are gzip files... bugzilla insists in keeping mime-type text/plain even when I set it to application/x-gzip
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 on attachment 139005 [details] Opencore AMR plugins, patch for -ugly Wrong attachment mime type was because "patch" flag was set.
Created attachment 139161 [details] [review] Opencore AMR plugins, patch for -bad
Created attachment 139162 [details] [review] Opencore AMR plugins, patch for -ugly
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.
Is it planned to add a amrwb encoder to the opencore library? Whatever, I'll take a look at those patches :)
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.