GNOME Bugzilla – Bug 567660
[API] need a stream volume interface for sinks that do volume control
Last modified: 2009-09-15 18:03:56 UTC
Right now a few sinks implement a "volume" property. This should be part of of a proper interface. Also, that interface should also provide a boolean "mute" in addition (and independantly) of the double "volume". Also, instead of using a linear factor for volume it might be a good idea to use hundreds of dB there (like everyone else). Finally, if you stick with linear for the volume then don't limit the factor to 0..10. but to at least 60dB (i.e. 1000).
That interface should probably contain a property to select the scale or some helper functions to convert from/to other scales.
Created attachment 142979 [details] [review] 0001-interfaces-API-Add-GstStreamVolume-interface.patch GstStreamVolume interface
Created attachment 142980 [details] [review] 0002-volume-Implement-GstStreamVolume-interface.patch Patch for volume
Created attachment 142981 [details] [review] 0003-playbin-2-Implement-GstStreamVolume-interface.patch Patch for playbin(2)
Created attachment 142982 [details] [review] 0001-pulsesink-Implement-GstStreamVolume-interface.patch Patch for pulsesink
In 0.11 playbin(2) should probably only use elements that implement the GstStreamVolume interface as volume elements and not all elements that have a "volume" property.
no mute property?
(In reply to comment #7) > no mute property? Right, next patch contains a mute property :)
Created attachment 142985 [details] [review] 0001-interfaces-API-Add-GstStreamVolume-interface.patch
Created attachment 142986 [details] [review] 0002-volume-Implement-GstStreamVolume-interface.patch
Created attachment 142987 [details] [review] 0003-playbin2-Implement-GstStreamVolume-interface.patch
One thing to keep in mind: if you define the interface like this it is only implementable by sinks that output to some software sound service, or to very few hardware devices. The reason for that is that not for all hardware we actually know the volume factor each possible volume step refers to. For example Bluetooth audio devices generally don't communicate which volume step means which amplification. They just offer an opaque integer in a range 0..15 where 0 is somehow less loud than 15, but that's all we know. Something similar is true for many USB devices and so on. Of course, this interface is about stream, not device volumes, so this probably isn't a real problem. And if some day we want to support 'opaque' stream volumes then we could still add another interface for that. But all I wanted to make sure is that this is kept in mind. Two comments though: it might be worth adding comments that the dB scale you chose is for amplitude, not power. (i.e. 20*log10, not 10*log10). Also, you might want to add a comment making clear that the cubic scale should be used for presentation in the UI. And referring to http://www.robotplanet.dk/audio/audio_gui_design/ or http://lists.linuxaudio.org/pipermail/linux-audio-dev/2009-May/thread.html#23151 might be a good idea, too.
Created attachment 142988 [details] [review] 0001-interfaces-API-Add-GstStreamVolume-interface.patch
Created attachment 142990 [details] [review] 0001-pulsesink-Implement-mute-property.patch
Created attachment 142991 [details] [review] 0002-pulsesink-Implement-GstStreamVolume-interface.patch
(In reply to comment #12) > One thing to keep in mind: if you define the interface like this it is only > implementable by sinks that output to some software sound service, or to very > few hardware devices. The reason for that is that not for all hardware we > actually know the volume factor each possible volume step refers to. For > example Bluetooth audio devices generally don't communicate which volume step > means which amplification. They just offer an opaque integer in a range 0..15 > where 0 is somehow less loud than 15, but that's all we know. Something similar > is true for many USB devices and so on. > > Of course, this interface is about stream, not device volumes, so this probably > isn't a real problem. And if some day we want to support 'opaque' stream > volumes then we could still add another interface for that. But all I wanted to > make sure is that this is kept in mind. Sure, for that stuff we already have the GstMixer interface. This is only for stream volumes. > Two comments though: it might be worth adding comments that the dB scale you > chose is for amplitude, not power. (i.e. 20*log10, not 10*log10). Good idea, actually I wasn't sure if I should take amplitude or power. What would you suggest to use? > Also, you > might want to add a comment making clear that the cubic scale should be used > for presentation in the UI. And referring to > http://www.robotplanet.dk/audio/audio_gui_design/ or > http://lists.linuxaudio.org/pipermail/linux-audio-dev/2009-May/thread.html#23151 > might be a good idea, too. That's already mentioned in the docs :)
Ok, Lennart said that amplitude is used by most other audio applications (and PA) and in the end it only differs in a constant factor of 2. commit 88bace6e3c82eec7e131ac1ee40c7ad021ed13a8 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Fri Sep 11 15:11:41 2009 +0200 playbin2: Implement GstStreamVolume interface commit de89db85e53e7dc5afeb57c039113b64cdaa44f7 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Fri Sep 11 15:04:42 2009 +0200 volume: Implement GstStreamVolume interface commit 92598bb2536bad3cbb3dfca92fec8941bf1deeaf Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Fri Sep 11 14:54:17 2009 +0200 interfaces: API: Add GstStreamVolume interface Fixes bug #567660.
Do we also want/need a way to notify the app of changes in the stream volume, e.g. via messages? Or do we just use notify::volume with all the threading issues involved?
is this somethings that should be implemented on e.g. pulsesrc/pulsesink as well? I totally missed the bug ...
(In reply to comment #18) > Do we also want/need a way to notify the app of changes in the stream volume, > e.g. via messages? Or do we just use notify::volume with all the threading > issues involved? We could, similar to the mixer interface, provide some messages and a function to post/parse them. Good idea. But that's for 0.10.26 then I guess. Or do you think it's an essential part of this interface and should be added ASAP? (In reply to comment #19) > is this somethings that should be implemented on e.g. pulsesrc/pulsesink as > well? I totally missed the bug ... On pulsesink yes (and there it is implemented already). Not sure if this should be implemented on pulsesrc too though (does pulse also support stream volumes on sources? If it does: yes it should be implemented).
> We could, similar to the mixer interface, provide some messages and a function > to post/parse them. Good idea. > > But that's for 0.10.26 then I guess. Or do you think it's an essential part of > this interface and should be added ASAP? I don't think it's a blocker, no.
(In reply to comment #20) > On pulsesink yes (and there it is implemented already). Not sure if this should > be implemented on pulsesrc too though (does pulse also support stream volumes > on sources? If it does: yes it should be implemented). We currently do not support stream volumes for recording. However this is likely going to change. In conjunction with flat volumes they actually make a lot of sense. And given that the revised flat vol logic is now apparently well liked by people I think we should extend it for record streams, too.
(In reply to comment #22) > (In reply to comment #20) > > > On pulsesink yes (and there it is implemented already). Not sure if this should > > be implemented on pulsesrc too though (does pulse also support stream volumes > > on sources? If it does: yes it should be implemented). > > We currently do not support stream volumes for recording. However this is > likely going to change. In conjunction with flat volumes they actually make a > lot of sense. And given that the revised flat vol logic is now apparently well > liked by people I think we should extend it for record streams, too. By 'we' I meant PA btw, just to be clear...