GNOME Bugzilla – Bug 391543
lame should add a tag with the chosen bitrate
Last modified: 2011-05-18 13:17:54 UTC
the CHECK_AND_FIXUP_BITRATE macro is buggy and for bitrate <= 64 kbps it always end up rounding it up to the first 64 multiple, hence 64.
while i m on lame, it would be nice to show the bitrate value in the src caps when using CBR encoding, that would allow muxers to store the bitrate correctly. It would also be nice to provide a way to force the output sample rate instead of always letting lame select it for us.
Created attachment 79142 [details] [review] Proposed patch fixing all those issues + Add the bitrate to the caps in CBR. + Fix the bitrate check macro. + add an out_samplerate property.
Looks good to me, but there are two small things I wonder about: - wouldn't "the GStreamer way" to force a specific output sample rate be filter caps after the lame element rather than a property? - why the bitrate in the caps and not in a tag event? Do we do it like this elsewhere as well?
+1 on not adding a property to lame just to force outgoing sample rate - this is waht filtercaps are for. Not sure about bitrate in the caps; I think it's ugly to communicate with a muxer this way - caps really are a way for elements to discuss together what they can and are going to do. bitrate is not really a property of the stream the way all the other caps are. Someone else should decide though.
Woah, that lame bug was nasty. Not being able to do 96 kbit/sec is silly :) I commited a partial and updated patch for that issue to CVS, and improving on the reporting, giving a WARNING message to the pipeline. We still need to decide how to inform the application of the bitrate chosen in some way.
I'd say lets use tags to inform the app: #define GST_TAG_BITRATE #define GST_TAG_NOMINAL_BITRATE #define GST_TAG_MINIMUM_BITRATE #define GST_TAG_MAXIMUM_BITRATE
bitrate must be an element configure parameter, just like the other gazilion config options in lame. Caps are strictly for letting the peer element know about the media type, not its configuration (which could be in the data itself) or the parameters used for creating this file.
bitrate is a parameter already. the tag should be use to inform about which bitrate actualy got used.
Ok, this was fixed after a few years now :) But only for the new lamemp3enc element, the lame element is deprecated and will disappear in 0.11 anyway. commit b0e7e273656ad49e7b5f91e369f74a315182bbe2 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Wed May 18 14:49:17 2011 +0200 lamemp3enc: Post CODEC and BITRATE tags Also filter any CODEC/AUDIO_CODEC tags from incoming tag events. Fixes bug #391543.