GNOME Bugzilla – Bug 740891
opusenc: replace "audio" property with new "audio-type" property
Last modified: 2015-01-29 13:39:59 UTC
The audio property is badly named. I think it should be rename voice, as it seems to enable low/high pass filter to enhance voice (deteriorating anything else).
(and flip the meaning of its boolean value)
I propose to make it an enum :)
And it shouldn't be 'audio' but 'music' perhaps?
I like the idea of an enum, something like target-audio, or audio-type, and values would be music and voice. Music would have been better then audio for the same meaning I think. It's all very subjective.
Fair point, though "audio" really means "generic audio" here, not music. IIRC Opus has voice specific optimizations, but no particular music optimizations. So maybe an audio-type-hint with "speech" and "generic" ?
Created attachment 295680 [details] [review] change audio property to audio-type My proposed shed color.
Comment on attachment 295680 [details] [review] change audio property to audio-type >+ {OPUS_APPLICATION_AUDIO, "Generic audio", "generic"}, >+ {OPUS_APPLICATION_VOIP, "Voice", "voip"}, Let's not use "voip" please, it should be "voice" also in this case. Although Speech/speech is perhaps better, I don't know. >- g_object_class_install_property (gobject_class, PROP_AUDIO, >- g_param_spec_boolean ("audio", "Audio or voice", >- "Audio or voice", DEFAULT_AUDIO, >- G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); >+ g_object_class_install_property (gobject_class, PROP_AUDIO_TYPE, >+ g_param_spec_enum ("audio-type", "What type of audio to optimize for", >+ "What type of audio to optimize for", GST_OPUS_ENC_TYPE_AUDIO_TYPE, >+ DEFAULT_AUDIO_TYPE, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); Let's deprecate the old name, that will make GLib throw warnings, then remove it in 1.7 after the 1.6 release so people have some time to update their code. Otherwise good to go in imho.
Created attachment 295733 [details] [review] change audio property to audio-type With requested changes.
Comment on attachment 295733 [details] [review] change audio property to audio-type >+ {OPUS_APPLICATION_VOIP, "Speech", "speech"}, Fwiw, I don't mind if it's voice or speech, I just didn't like the Voice/voip mismatch. (Arguably voice is the more generic term :)) >- g_param_spec_boolean ("audio", "Audio or voice", >- "Audio or voice", DEFAULT_AUDIO, >+ g_param_spec_boolean ("audio", >+ "Audio or voice (obsolete, use audio-type)", >+ "Audio or voice (obsolete, use audio-type)", DEFAULT_AUDIO, > G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); | G_PARAM_DEPRECATED so that GLib generates warnings for us if the old property is used. Please push then. Thanks!
Nice, I did not know of that flag. However, while gst-inspect shows "deprecated" in the flag list, I don't get glib warnings when using the property. I could have too old a glib I suppose. Should I remove my manual warnings anyway then ?
Ah well, just leave it in then, can't hurt to get two warnings. Looks like in older GLibs a warning was only emitted if G_ENABLE_DIAGNOSTIC was set to 1, now (since mid-2014) it's emitted unless that's set to 0 (see bug #732184).
Created attachment 295754 [details] [review] change audio property to audio-type With changes from above.
commit 5c44bceeb375e22720620d4d7acda7c054dde5ef Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Wed Jan 28 16:43:59 2015 +0000 opusenc: change audio property to audio-type This is now an enum with values generic (default) and voice. https://bugzilla.gnome.org/show_bug.cgi?id=740891