GNOME Bugzilla – Bug 584252
enhancements to OSSv4 plugin
Last modified: 2010-07-02 19:18:10 UTC
Please describe the problem: I'm the lead developer for Solaris' Boomer project, and as such, I've developed a significant set of enhancements to the OSSv4 plugin in gst-plugins-bad. These enhancements make Gstreamer work much better on Solaris with Boomer (OpenSolaris build 115 and later), but also make it work better with stock OSSv4 from 4Front (verified by Dev Mazumdar at 4Front.) Most of the changes involve general code cleanup and quality improvements, but I've also significantly revamped the way the audio controls are presented, so that gnome-volume-control presents a much clearer and more consistent picture of the controls available on an audio device. This fixes incorrect handling of the MUTE option, and incorrect placement of some of the controls on gnome-volume-control. It also significantly enhances the ability to localize GStreamer controls on OSSv4 systems. Steps to reproduce: 1. Run gstreamer with gnome-volume-control using the OSSv4 plugin 2. Notice that some controls have improper mute values, are not localized, etc. 3. Actual results: Expected results: Does this happen every time? Other information: The detailed changes in behavior are too many to list here. This is basically a major overhaul of the entire plugin. Additionally Brian Cameron and I have worked on this, and I believe that Brian and I will be committing to long term sustaining, and working to move this out of -bad. Finally, I want to note that Dev Mazumdar at 4Front is pleased with these changes -- they work better with stock OSSv4 than the previous code, so the code is an unqualified improvement.
Created attachment 135597 [details] [review] My patches for OSSv4 plugin This patch has been in use at Sun Microsystems, and represents the code that we would like to see in place on official gstreamer for Solaris.
Hi, thanks for the patch. Do you by any chance have a set of cumulative patches, like a git/mercurial branch or so? Or is it just this giant patch? There seem to be quite a few cosmetic changes, and also broken indenting. It would be great if you could update your patch after running 'gst-indent *.c' on the changed files (required GNU indent to be installed). (At this point you'll probably also realise why there are those semicolons after those macros :))
Created attachment 135901 [details] [review] updated patch Note that many of the changes in this patch are related to adding new flags to gst-plugins-base. Refer: http://bugzilla.gnome.org/show_bug.cgi?id=570832 http://bugzilla.gnome.org/show_bug.cgi?id=571106 Now that these changes are upstream, it would be good to support the new flags in the OSSv4 plugin. I have attached an updated patch. I believe this updated patch cleans up the code so it conforms to the style and removes some unnecessary changes. Here is some detail on the changes: 1) oss4-audio.c is modified to simplify the fragsize computation. 2) oss4-mixer-slider.c is modified to support the new mixer-related flag and also contains this change: if (mute) { + /* make sure the current volume values get saved. */ + gst_oss4_mixer_slider_get_volume (s, s->volumes); volume = 0; } else { This change fixes a problem where the volumes weren't getting saved before muting for the first time. Calling gst_oss4_mixer_slider_get_volume makes sure the volumes are cached before setting to 0. Otherwise, when you mute and then unmute, the volume wouldn't get restored. 3) oss4-mixer.c contains improvements on how mixer labels are translated, the GST_OSS4_MIXER_WATCH_INTERVAL was changed from 500 to 150ms. the guess_master code was improved, and support for the new flags. 4) oss4-mixer-switch now supports the new mute-related flag.
I get some warnings compiling with this patch: oss4-mixer.c: In function `gst_oss4_mixer_control_get_translated_name': oss4-mixer.c:862: warning: 'num' might be used uninitialized in this function oss4-mixer.c: At top level: oss4-mixer.c:678: warning: 'gst_oss4_mixer_control_get_pretty_name' defined but not used oss4-audio.c: In function `gst_oss4_audio_set_format': oss4-audio.c:502: warning: 'fragsize' might be used uninitialized in this function The fixes for the last 2 are pretty obvious, but it's less obvious how to fix the first.
Btw, I'm not particularly in favour of reducing the poll interval of the notification thread from 500ms to 150ms in general. A patch that adjusts the interval dynamically (ie. wake up every 500ms or so by default, but more frequently if changes were detected recently) would be more acceptable imo. Even better would be if the OSS folks fixed this on their end so that we don't have to poll at all, but could just watch an fd or so for change notifications. This would all be much easier if you provided separate patches for separate changes btw. (git add -i comes in handy here).
I just talked with Garrett on the phone, and have some feedback about the two outstanding issues. Tim-Phillipp: Garret agreed that keeping the poll interval at 500ms is acceptable. Jan: if you look at the oss4-mixer.c code, in the gst_oss4_mixer_control_get_translated_name function you will notice that the num variable is only used in a call to g_snprintf call that looks like this: g_snprintf(name, sizeof (name), fmtbuf, _(labels[i].label), num); Note that fmtbuf by default is "%s", so the num argument is ignored in this case. However when the code falls into the previous if-case where num is set, then fmtbuf is also changed to "%s %n". So, in other words, the num variable in the g_snprintf call is only used if it falls into the if-block where num is set to a value. To make the compiler happier, and avoid the warning, it would be okay to just initialize it to -1 at the beginning of the function. Jan, can you go ahead and make these two minor changes (reset the poll back to 500ms and initialize the num variable to -1) before you commit? Or would you like me to regenerate the patch with these two changes? Thanks, Brian
No worries, I'll fix it up in the morning. The Bad plugins are frozen for release at the moment - I'll commit it after the thaw.
After a reminder from Brian, I finally remembered to actually push the commit to master: commit fac1dbab6e19d5a79a417864f496d2be86250bbb Author: Garrett D'Amore <garrett.damore@sun.com> Date: Wed Jun 10 19:21:21 2009 +0100 oss4: Enhancements to the mixer and audio output Code cleanups, general improvements, support for the new mixer flags in latest gst-plugins-base. Fixes: #584252 Patch By: Brian Cameron <brian.cameron@sun.com> Patch By: Garrett D'Amore <garrett.damore@sun.com>