GNOME Bugzilla – Bug 614305
[PLUGIN-MOVE] oss4 should be moved to good
Last modified: 2010-07-02 19:16:31 UTC
After reviewing the OSS4 plugin in gst-plugins-bad, I see that it has the same degree of documentation that the OSS plugin in gst-plugins-good has. From GStreamer's perspective, these plugins do not expose different interfaces - instead they just work with different versions of OSS. The OSSv4 plugin documentation does have minor changes to the docs to highlight that this plugin is OSSv4 specific. This plugin has been used in Solaris and OpenSolaris now since March 6th, 2009 (a little over a year) and we have not had any users complain about it not working properly since the 07-15-2009 commit where needed features were added (note that Solaris/OpenSolaris has been applying that change as a patch internally since March 6th, 2009). So I think it is fair to say that this plugin is reasonably stable and working at this point. Changes to the code since the 07-15-2009 commit seem mostly trivial cleanups. Can this plugin be moved to good? If not, what work remains to be done?
Do we need two copies of the plugin? Doesn't the oss4 plugin work with oss2 drivers?
> Doesn't the oss4 plugin work with oss2 drivers? It doesn't, on purpose (it checks the version and will error out for older drivers and even old oss4 drivers). It uses the new oss4 API for a number of things like device selection/discovery and the mixer stuff (completely new api here). > Do we need two copies of the plugin? It's probably possible to combine the two plugins, but I think it's undesirable. I'd rather keep them separate. Makes for cleaner and more maintainable and more debuggable code. I have no objections to the OSS4 plugin being moved to good; I guess it's as good as it's going to get given the API (but I would say that), so there's no reason not to let it be in good company with the esd plugin and the old oss plugin. The checklist for plugin moves can be found here: http://cgit.freedesktop.org/gstreamer/gstreamer/tree/docs/random/moving-plugins
I discussed this with Garrett D'Amore and he said that some of the newer mixer interfaces the OSSv4 plugin uses are not available in OSSv3. I have reviewed the checklist and I believe that this plugin meets all the requirements. The coding suggestions seem to be implemented by this plugin, gst-inspect output looks good, strings for translations seem right, and the documentation seems reasonable to me. This plugin does not really come with any "tests", however it seems that all src/sink/mixer plugins in gst-plugins-good (oss, osxaudio, osxvideo, sunaudio, esd) also do not have any special tests. I assume this is because it is straightforward to test these sorts of plugins using gst-launch, gnome-volume-control, etc. But if there is any work that should be done as a part of its move to good, please let me know since I would like to invest the time to get this plugin blessed and released in good.
Something that should be done before this gets moved to -good: Since OSS4 conceptually has per-app/stream volumes and is capable of doing the volume handling itself in the backend, oss4sink should implement the GstStreamVolume interface and implement a "volume" and "mute" property, so it works well with playbin2/totem etc.
And btw, there's an oss4 test in -bad/tests/icles/test-oss4.c.
Thanks for the review. I think I need some help, though. Could you explain how to test this interface once it is written? For example, how in totem can you make use of this or test it? The following GStreamer docs explain the interface but does not really provide any tips on how to actually make use of it: http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-libs/html/gst-plugins-base-libs-gststreamvolume.html#GstStreamVolume-struct Is there an example of a well written plugin that makes use of this interface I could use as an example? Looking at the gst-plugins-base and gst-plugins-good code, the only reference I see to a plugin making use of it is the pulsesink plugin. However, I see no reference to the string "GstStreamVolume" in the pulse code. It does seem to implement the PROP_VOLUME and PROP_MUTE properties, though. Is there any documentation which explain how to actually use it? Can you test it using gst-launch commands, for example? Running gst-inspect on decodebin2, I do not see any obvious properties to control the stream volume/mute settings. You say that we need to add the VOLUME and MUTE properties to the OSSv4 plugin, so does this mean that this feature is implemented just by the plugin having these properties and you do not really have to implement the GstStreamVolume interfaces in the OSSv4 plugin code?
> Could you explain how to test this interface once it is written? For example, > how in totem can you make use of this or test it? The following GStreamer > docs explain the interface but does not really provide any tips on how to > actually make use of it: > > http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-libs/html/gst-plugins-base-libs-gststreamvolume.html#GstStreamVolume-struct playbin2 (and hence also recent versions of totem) will/should use it when changing the volume or muting audio. So with a recent totem you can just test whether the right code paths in oss4sink are hit. Also, when you look at the oss4 mixer (the OSS one, not a gstreamer-based one), there should be a mixer control for each application that uses OSS4, so there should be a mixer slider for totem then. If the interface is used correctly, then you should see the totem slider in the OSS4 mixer move up and down when you change the volume in totem. If the volume changes without the slider moving, that means that playbin2's internal volume element does the volume control. > Is there an example of a well written plugin that makes use of this interface I > could use as an example? Looking at the gst-plugins-base and gst-plugins-good > code, the only reference I see to a plugin making use of it is the pulsesink > plugin. However, I see no reference to the string "GstStreamVolume" in the > pulse code. Look for: static const GInterfaceInfo svol_iface_info = { NULL, NULL, NULL }; g_type_add_interface_static (type, GST_TYPE_STREAM_VOLUME, &svol_iface_info); (I don't think you need the additional GstImplementsInterface stuff that's going on there, I don't know why that's there..). The interface has no vfuncs/methods to implement, so that's all there is to it besides adding and implementing the properties. > Is there any documentation which explain how to actually use it? Not that I know of. If totem isn't enough, You can always write some code that calls the interface methods mentioned in the API.. > Can you test it using gst-launch commands, for example? No. > You say that we need to add the VOLUME and MUTE properties to the OSSv4 plugin, > so does this mean that this feature is implemented just by the plugin having these > properties and you do not really have to implement the GstStreamVolume > interfaces in the OSSv4 plugin code? Technically you could probably get away with just implementing the properties and not the interface. However, the interface should really also be added. The reason for that is that it makes the semantics of the volume property clear (linear volume, range etc.).
Created attachment 158922 [details] [review] patch implementing GstStreamVolume Here is a patch that implements the GstStreamVolume interface for the OSSv4 plugin. Based on my testing it seems to work well. I do see that the properties are being used to get and set the volume properly when using the plugin built with this patch. With this patch, can the OSSv4 plugin be moved to gst-plugins-good? --- One thing I don't like about using the GstStreamVolume interface is that when using the OSSv4 plugin with this change, the volume decreases much more rapidly when I move the volume slider down in programs like totem or rhythmbox. From looking at the code in a debugger, I can see the values go down quickly because of the conversion to/from "GST_STREAM_VOLUME_FORMAT_CUBIC" which is used by totem & rhythmbox. Since the volume slider doesn't decrease volume so much when you slide it down before the OSSv4 plugin implemented this interface, I assume that the GST_STREAM_VOLUME_FORMAT_CUBIC was being ignored before. But if the GStreamer community feels that it is better for the volume to decrease rapidly like this, then I guess this isn't a problem. I notice that the gst-plugins-base documentation recommends that GUI's use CUBIC.
The patch looks good in general but your forgot to add the implementsinterface interface to the type, you only added the stream volume interface. Using cubic volumes for GUIs is definitely a good idea, it "feels" more natural and also all GNOME applications are currently doing that (and all pulseaudio applications, you don't use PA on Solaris?).
> but your forgot to add the implementsinterface interface to > the type, you only added the stream volume interface. Well, that's what I suggested above. What's the point of the implements interface here? We're trying to get rid of that horrible piece of API.
(In reply to comment #10) > > but your forgot to add the implementsinterface interface to > > the type, you only added the stream volume interface. > > Well, that's what I suggested above. What's the point of the implements > interface here? We're trying to get rid of that horrible piece of API. Well, he added the implementations of that interface with the patch :) Either they should be removed or used properly (but I'd prefer the removal too).
Ah, yes (hadn't looked at the patch yet). Some more comments on the patch: - the SET/GET ioctls should be moved into a helper function - we probably need to add a lock to make sure the SET/GET volume ioctls are not called at the same time (from the application thread) as the SNDCTL_DSP_GETODELAY ioctl (which would be called from the streaming thread), since last time I checked OSS4 didn't handle ioctls on the same fd from multiple threads very well (understatement). - I think the stream volume should be stored as a single mono volume value internally, you can then construct the VOL | (VOL << 8) thing later for the ioctl if needed.
Created attachment 159111 [details] [review] updated patch to add GstStreamVolume Here is an updated patch which does the following: 1) Now get/set volume/mute is done with helper functions. 2) Added GST_OBJECT_LOCK/GST_OBJECT_UNLOCK calls surrounding ioctl calls. 3) Added the g_type_add_interface_static () call for GST_TYPE_IMPLEMENTS_INTERFACE. I think this fixes the major issues raised about this patch. Is there any more work that needs to be done? Note that I did not change mute_volume so that it is stored as a single mono volume value internally. This seems a bad idea to me for several reasons. This value is just loaded from the ioctl when muted, and then when unmuting, the code just put the same value back via the ioctl call. This is much easier than doing extra calculations. The current code avoids doing extra calculations and only actually sets the value via code like "oss->mute_volume = val | (val << 8)" when initializing it in gst_oss4_sink_class_init and when setting it to 100 if the saved mute volume happens to be 0. Saving the value internally as a single mono value would require doing extra calculations when getting and setting the value, which seems better to just avoid. If you feel strongly about this, though, I can change the code.
*ping*
> Created an attachment (id=159111) [details] [review] > updated patch to add GstStreamVolume This was missing the header file change. > 1) Now get/set volume/mute is done with helper functions. Ok, I've moved the getters up to the setters to they're closer together. Also, I've changed the error code paths in the getters a bit so they don't operate on invalid/undefined values. > 2) Added GST_OBJECT_LOCK/GST_OBJECT_UNLOCK calls surrounding ioctl calls. Great. > 3) Added the g_type_add_interface_static () call for > GST_TYPE_IMPLEMENTS_INTERFACE. I've removed all the GstImplementsInterface stuff. It's not needed here and we want to get rid of this horrible interface, so no point adding it here for no reason. > I think this fixes the major issues raised about this patch. Is there any more > work that needs to be done? Nothing I can think of right now. I'm happy to move it as it is, although technically we still need someone to review the code for sanity and compliance with the moving-plugins guidelines. Not sure if Sebastian or anyone else have already done that. > Note that I did not change mute_volume so that it is stored as a single mono > volume value internally. This seems a bad idea to me for several reasons. (...) Makes sense.
Comment on attachment 159111 [details] [review] updated patch to add GstStreamVolume commit 8f12893c9173372794325403560fe980ad59ea3f Author: Brian Cameron <brian.cameron@oracle.com> Date: Thu Apr 29 13:18:58 2010 +0100 oss4sink: implement GstStreamVolume interface and add mute and volume properties OSS4 supports per-stream volume control, so expose this using the right API, so that playbin2 and applications like totem can make use of it (instead of using a volume element for volume control). Fixes #614305.
Thiagos has had a glance at this as well, so moved to -good: commit e800ba112b29f4a9ac4465fd0676a8e3bb701d88 Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Tue May 11 15:33:54 2010 +0100 Remove oss4 plugin It has been moved to gst-plugins-good. See #614305. commit 1732ce033a71ed3d976f038b0e6f680968862711 Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Tue May 11 16:08:21 2010 +0100 Move oss4 plugin from -bad to -good Hook up build infrastructure, docs and tests. Fixes #614305.