GNOME Bugzilla – Bug 494528
Simplifications of the LAME plugin
Last modified: 2009-05-10 21:48:13 UTC
Current Lame plugin is offering many many options, including some deprecated ones and potentially harmful ones. I think this is mainly our (Lame devs) fault, as Lame provides many options to the user that should never had been provided outside of debug builds. Since a few years, we are trying to simplify things a lot, and are encouraging third-party devs to provide a simple user interface instead of the "usual" options-cluttered one (http://lame.sourceforge.net/lame_ui_example.php). I think that it might be a good thing to simplify the current Gstreamer Lame plugin, so I am going to provide a list of comments there. Please tell me if this should be split into several entries. #outdated options, not doing anything: lowpass_width/highpass_width/cwlimit Those are deprecated, and are just preserved within current Lame API in order to not break external apps that might still be using them. #not doing anything usefull: gboolean extension (ARG_EXTENSION) This one is just switching a bit within the bistream, for which we never found any use. #not needed anymore: preset(ARG_PRESET) Not needed since Lame 3.94 (released in 2003). "Presets" are now always automatically used. strict_iso(ARG_STRICT_ISO) Always used #potentially (mostly always) harmfull for quality, I think they should not be offered to the user: force_ms disable_reservoir ath_only ath_short no_ath ath_type ath_lower allow_diff_short no_short_blocks #potential problems: *Sink pad is audio/x-raw-int. Why not using audio/x-raw-float as the Vorbis encoder? *GST_TYPE_LAME_MODE "auto" is deprecated (better not provide it), "Stereo" should not be used (joint should be preferred), "Dual Channel" would probably be better understood if it was named "Dual Content / Dual Channel". *GST_TYPE_LAME_QUALITY Default Lame value is 2 (and not 5) since several years ago. I agree that this one is a bit tricky for you, as current Lame API doesn't really tell you about the default value. *DEFAULT_MIN_VBR_BITRATE Our default is 32 for mpeg1 and 8 for mpeg2. Gstreamer plugin default value is 112, which is way too high. *DEFAULT_MAX_VBR_BITRATE Our default is 320 for mpeg1 and 160 for mpeg2. Current plugin value of 160 is way too low when using mpeg1. I understand that it's probably this way because this value works both in mpeg1 and mpeg2 case, so solving this would be tricky. *on GST_EVENT_FLUSH_STOP, you are using a buffer of 7200. Why not always using the same buffer value, both when encoding and on closing? (mp3_buffer_size = 1.25 * num_samples + 7200) 7200 will sometimes not be big enough.
Thanks for the summary. It's a bit hard to change certain things in the plugin, like default values and sink caps, since we need to maintain a certain degree of backwards compatibility. However, what could easily be done is to add, say, new lamemp3enc and lamemp2enc elements with new/cleaned up options and other sink caps.
I understand the unfortunate need to maintain backward compatibility, it this is also why many options are still offered within the libmp3lame interface, even if some don't do anything anymore and some others are likely to be harmfull. Considering this, your suggestion of a new, to be cleaned, lamemp3enc element seems to be a good idea. I could probably provide a few patches, but can't maintain this one. note: Lame only encodes in mp3, so no lamemp2enc (probably toolamemp2enc or twolamemp2enc)
This is actually a pretty serious issue because default values are set for a lot of these deprecated/harmful switches inside the gstreamer Lame element. For example, if the user tries to emulate standalone Lame with -V 4 by giving the gstreamer Lame element vbr-quality=4, the gstreamer Lame element will pass liblame a full complement of values for these deprecated/harmful switches, completely ruining encoding quality (not to mention the user's original intent). See also https://bugs.launchpad.net/sound-juicer/+bug/195483
Hi, I'm a developper of gnac, an audio converter. Gnac strongly depends on the lame plugin to encode mp3s (as any application that uses Gstreamer). As mp3 is the most used format for portable players and for music distribution, we are facing a major issue... What are plans about this bug? Can we expect some update soon? David
Implementing this shouldn't be a problem after someone evaluated which settings should be exported by a new lamemp3enc element. I could probably implement something like the LAME UI suggestion later today and attach it to this bug...
Created attachment 133580 [details] [review] patch
The patch above adds this new element... it has exactly the same properties as the screenshot of the LAME GUI. Documentation will be updated a bit later and locally I have the documentation integrated into the build system already. Would be nice to get this reviewed and into 0.10.12...
> * on GST_EVENT_FLUSH_STOP, you are using a buffer of 7200. Why not always using > the same buffer value, both when encoding and on closing? (mp3_buffer_size = > 1.25 * num_samples + 7200) > 7200 will sometimes not be big enough. Gabriel, what should be used for the number of samples? lame_get_size_mp3buffer() can't be used because it doesn't include the cached raw samples that would be encoded by lame_encode_flush()...
(In reply to comment #8) > > * on GST_EVENT_FLUSH_STOP, you are using a buffer of 7200. Why not always using > > the same buffer value, both when encoding and on closing? (mp3_buffer_size = > > 1.25 * num_samples + 7200) > > 7200 will sometimes not be big enough. > > Gabriel, what should be used for the number of samples? > lame_get_size_mp3buffer() can't be used because it doesn't include the cached > raw samples that would be encoded by lame_encode_flush()... > Don't know what I was thinking. Indeed, in case of flush event, you don't have any input sample. You can then call lame_encode_flush in a loop, untill it returns 0.
(In reply to comment #7) > The patch above adds this new element... it has exactly the same properties as > the screenshot of the LAME GUI. > Would be nice to get this reviewed and into 0.10.12... Since Lame 3.98 (released in July 2008) the default VBR mode is now what was previously known as "fast" VBR, so you might want to drop the ARG_FAST_VBR property.
Created attachment 133632 [details] [review] patch-flush flush the encoder as long as no data is left (patch relative to previous one)
(In reply to comment #10) > (In reply to comment #7) > > The patch above adds this new element... it has exactly the same properties as > > the screenshot of the LAME GUI. > > > > Would be nice to get this reviewed and into 0.10.12... > > Since Lame 3.98 (released in July 2008) the default VBR mode is now what was > previously known as "fast" VBR, so you might want to drop the ARG_FAST_VBR > property. Which vbr mode is this? vbr_mtrh or vbr_rh? Would there be a problem if the same mode is used for lame 3.98 and previous versions?
(In reply to comment #9) > (In reply to comment #8) > > > * on GST_EVENT_FLUSH_STOP, you are using a buffer of 7200. Why not always using > > > the same buffer value, both when encoding and on closing? (mp3_buffer_size = > > > 1.25 * num_samples + 7200) > > > 7200 will sometimes not be big enough. > > > > Gabriel, what should be used for the number of samples? > > lame_get_size_mp3buffer() can't be used because it doesn't include the cached > > raw samples that would be encoded by lame_encode_flush()... > > > > Don't know what I was thinking. Indeed, in case of flush event, you don't have > any input sample. > You can then call lame_encode_flush in a loop, untill it returns 0. This doesn't work, lame_encode_flush() will always return 208 bytes for all calls but the first call... forever :)
(In reply to comment #13) > This doesn't work, lame_encode_flush() will always return 208 bytes for all > calls but the first call... forever :) > Indeed. Please hold on a bit, I'm going to properly check it, and I'll get back to you.
(In reply to comment #12) > (In reply to comment #10) > > (In reply to comment #7) > > > The patch above adds this new element... it has exactly the same properties as > > > the screenshot of the LAME GUI. > > > > > > > Would be nice to get this reviewed and into 0.10.12... > > > > Since Lame 3.98 (released in July 2008) the default VBR mode is now what was > > previously known as "fast" VBR, so you might want to drop the ARG_FAST_VBR > > property. > > Which vbr mode is this? vbr_mtrh or vbr_rh? Would there be a problem if the > same mode is used for lame 3.98 and previous versions? > It is vbr_mtrh. There should not be any real issue if this mode is also used with older lame versions, from about 3.96 (released in 2004). Alternatively, if you are statically linking with libmp3lame, you can also use the vbr_default value.
Certainly a much nicer API than that of the 'current' lame plugin I'm not entirely convinced the property names can't be improved upon some more, but maybe that's just me. E.g. "target" seems a bit nondescriptive by itself, and "encoding-engine-quality" is a bit long for my taste (and maybe too easily mistaken for encoding quality?). Feel free to ignore me on this though, I don't really have any better suggestions either at the moment. Didn't look at the code, but I take it is mostly identical to the existing lame element, right?
Yes, I've committed it now with an additional note that the encoding-engine-quality doesn't affect the bitrate. For the FLUSH/EOS issue I'll clone this bug later...
OK, I've been playing around with it a bit. Took a sound sample and encoded it with standalone lame and the new lamemp3enc gstreamer element under a number of common scenarios. Each time, the outputs were bitwise identical except that lame produces a Xing header, while the lamemp3enc gstreamer element occupied its space with zeros (this has always been the case and I assume it is was done "by design"). AFAIAC, lamemp3enc passes with flying colors. Sebastian, you made a lot of people (including me) very happy by fixing this longstanding annoyance! Can't wait until I see it in my favorite distro's official repositories ;) Cheers!
Thanks, that's great to hear :) GStreamer doesn't use lame to write the xing header, for this you can use the xingmux element after lame.
(In reply to comment #18) > Each time, the outputs were bitwise identical except that > lame produces a Xing header, while the lamemp3enc gstreamer element occupied > its space with zeros (this has always been the case and I assume it is was done > "by design"). This is because libmp3lame would need to be able to seek back (rewind) within the stream to be able to write the vbr tag once encoding is finished, and this is not possible within GStreamer. The initial "padding" with zeros is the placeholder for the vbr tag. To avoid this initial placeholder, you can use lame_set_bWriteVbrTag. It would indeed be a nice idea to actually prevent those leading zeros.
(In reply to comment #20) > (In reply to comment #18) > > Each time, the outputs were bitwise identical except that > > lame produces a Xing header, while the lamemp3enc gstreamer element occupied > > its space with zeros (this has always been the case and I assume it is was done > > "by design"). > > This is because libmp3lame would need to be able to seek back (rewind) within > the stream to be able to write the vbr tag once encoding is finished, and this > is not possible within GStreamer. Actually this would be possible but we already have other means to produce a Xing header which simplifies the lame plugin ;) > The initial "padding" with zeros is the placeholder for the vbr tag. To avoid > this initial placeholder, you can use lame_set_bWriteVbrTag. > It would indeed be a nice idea to actually prevent those leading zeros. I've done that already earlier today :)