GNOME Bugzilla – Bug 744909
opusenc: cbr and constrained-vbr are confusing
Last modified: 2015-03-12 14:05:19 UTC
I am running the Ubuntu 14.04 package of GStreamer 1.2.4. I ran this command: gst-inspect-1.0 opusenc And saw this: cbr : Constant bit rate flags: readable, writable, changeable in NULL, READY, PAUSED or PLAYING state Boolean. Default: true constrained-vbr : Constrained VBR flags: readable, writable, changeable in NULL, READY, PAUSED or PLAYING state Boolean. Default: true It seems like both of these being set to "true" would contradict eachother. This fact is the bug I am reporting. Furthermore, I viewed the master branch, and saw this: http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/opus/gstopusenc.c 161 #define DEFAULT_CBR TRUE 162 #define DEFAULT_CONSTRAINED_VBR TRUE
If CBR = TRUE, then VBR is disabled. That means contrained_vbr has no effect by default. I don't see any bug here. What is means if you set CBR = FALSE, you'll get VBR with contrained_vbr enabled by default. For sure, CBR property could have be made something like bitrate-control = VBR/CBR. It will remains that CBR or VBR specific properties only have effect if the specific mode is chosen.
Thank you for explaining this.
I think it's quite confusing and would argue it's a bug in our application interface (properties). I also wonder if constrained-vbr=true is a good default value. If someone explicitly disables the default constant bitrate, chances are that they want real VBR and not almost-cbr-but-not-quite VBR (i.e. constrained-vbr)
(In reply to Tim-Philipp Müller from comment #3) > I think it's quite confusing and would argue it's a bug in our application > interface (properties). It's not very good no. > > I also wonder if constrained-vbr=true is a good default value. If someone > explicitly disables the default constant bitrate, chances are that they want > real VBR and not almost-cbr-but-not-quite VBR (i.e. constrained-vbr) I would have to research what it means. Are you sure it's an almost cbr but not quite VBR option ?
That's how I interpret http://www.opus-codec.org/presentations/opus_ccbe2013.pdf slide 8. May well be wrong of course.
Ok, definitely, having two properties to actually turn on VBR is like Windows asking you if you are sure to be sure ;-P
FWIW, src/opus_encoder.c in libopus says: /* Makes constrained VBR the default (safer for real-time use) */ I'm guessing this is to avoid large bitrate spikes. As for the properties, how about an enum cbr, constrained-vbr, full-vbr ?
Created attachment 299198 [details] [review] replace properties with an enum No comments, so I guess nobody is against enough :) Here's the patch to do that.
Comment on attachment 299198 [details] [review] replace properties with an enum Works for me, thanks. I'd make the enum a typedef, and perhaps consistent with the GType name you've chosen. Do we know of any users of these properties, ie. should we perhaps deprecate them and keep them around for another cycle before removing them?
Created attachment 299202 [details] [review] replace properties with an enum With a typedef and previous properties back as deprecated.
Comment on attachment 299202 [details] [review] replace properties with an enum Ok (Perhaps also use the typedef'ed enum in the header then?)
Created attachment 299205 [details] [review] replace properties with an enum
commit 22d9c9f7635ad8be0e248d0fc63ea3063a6c8c96 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Thu Mar 12 12:49:40 2015 +0000 opusenc: replace cbr and constrained-vbr properties with an enum It was deemed confusing before. https://bugzilla.gnome.org/show_bug.cgi?id=744909