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 744909 - opusenc: cbr and constrained-vbr are confusing
opusenc: cbr and constrained-vbr are confusing
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-02-21 19:11 UTC by Mike DePaulo
Modified: 2015-03-12 14:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
replace properties with an enum (6.47 KB, patch)
2015-03-12 12:51 UTC, Vincent Penquerc'h
none Details | Review
replace properties with an enum (7.60 KB, patch)
2015-03-12 13:31 UTC, Vincent Penquerc'h
none Details | Review
replace properties with an enum (7.79 KB, patch)
2015-03-12 14:02 UTC, Vincent Penquerc'h
committed Details | Review

Description Mike DePaulo 2015-02-21 19:11:01 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
Comment 1 Nicolas Dufresne (ndufresne) 2015-02-21 19:21:07 UTC
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.
Comment 2 Mike DePaulo 2015-02-22 11:55:05 UTC
Thank you for explaining this.
Comment 3 Tim-Philipp Müller 2015-02-23 19:27:41 UTC
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)
Comment 4 Nicolas Dufresne (ndufresne) 2015-02-24 12:44:00 UTC
(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 ?
Comment 5 Tim-Philipp Müller 2015-02-24 12:54:38 UTC
That's how I interpret

 http://www.opus-codec.org/presentations/opus_ccbe2013.pdf

slide 8. May well be wrong of course.
Comment 6 Nicolas Dufresne (ndufresne) 2015-02-24 13:31:51 UTC
Ok, definitely, having two properties to actually turn on VBR is like Windows asking you if you are sure to be sure ;-P
Comment 7 Vincent Penquerc'h 2015-03-02 09:22:19 UTC
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 ?
Comment 8 Vincent Penquerc'h 2015-03-12 12:51:53 UTC
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 9 Tim-Philipp Müller 2015-03-12 12:56:33 UTC
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?
Comment 10 Vincent Penquerc'h 2015-03-12 13:31:36 UTC
Created attachment 299202 [details] [review]
replace properties with an enum

With a typedef and previous properties back as deprecated.
Comment 11 Tim-Philipp Müller 2015-03-12 13:53:13 UTC
Comment on attachment 299202 [details] [review]
replace properties with an enum

Ok (Perhaps also use the typedef'ed enum in the header then?)
Comment 12 Vincent Penquerc'h 2015-03-12 14:02:13 UTC
Created attachment 299205 [details] [review]
replace properties with an enum
Comment 13 Vincent Penquerc'h 2015-03-12 14:05:02 UTC
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