GNOME Bugzilla – Bug 321269
[sunaudio] port to GStreamer-0.10
Last modified: 2006-01-23 11:22:12 UTC
Attaching a patch to add sunaudio sink and mixer plugins to 0.9. I didn't convert the source plugin since looking at the code, it is nonfunctional. Have to work on adding this for real later. I'm gussing this should be added to gst-plugins-good, but if another module makes more sense, that's okay.
Created attachment 54657 [details] [review] patch to add sunaudio mixer and sink plugins. I haven't tested the mixer plugin yet, since talking to the GStreamer team on IRC, it seems there isn't a test tool yet for testing 0.9 mixer plugins. I'll test this when it is possible to do so. It compiles, and looks right to me.
Note, the sink plugin has been tested and works.
This looks a bit strange: the comment and the accepted range don't seem to match (and since 48000 is a pretty common sample rate, it'd be bad to needlessly reject it if the hardware does support it) + /* [5510,48000] seems to be a Solaris limit */ + "rate = (int) [ 5510, 44100 ], " I'm unsure of the appropriateness of this; shouldn't this (looking up env vars and setting properties as appropriate) be an app-level thing? If you think it's the right approach, that's ok. + audiodev = g_getenv ("AUDIODEV"); + if (audiodev == NULL) + audiodev = DEFAULT_DEVICE; Looks like you're leaking a pad template here. More generally, I haven't checked closely for memory leaks, you should be careful. + pad_template = gst_static_pad_template_get (&gst_sunaudiosink_factory); + caps = gst_caps_copy (gst_pad_template_get_caps (pad_template)); + + return caps; +} Why is it neccesary to open non-blocking? Does an open of the audio device sometimes block in solaris? There's an obvious race condition here; is it possible (following a non-blocking open) to convert the fd to blocking mode, rather than closing/reopening it? + /* First try to open non-blocking */ + fd = open (sunaudiosink->device, O_WRONLY | O_NONBLOCK); + + if (fd >= 0) { + close (fd); + fd = open (sunaudiosink->device, O_WRONLY); + } + The close/reopen on unprepare looks... strange. Why are you doing this? I think you half-copied this from osssink, which does this due to oddities in the OSS API that probably aren't shared by SunAudio. I'd guess you probably don't need to do anything in your unprepare function, though I might be wrong. Your reset() function looks incorrect. All you do here at the moment is close the device - and you don't check for that when your close() function is called. Doing nothing in this function may be sufficient (as in alsasink and osssink), I think. This generally looks good, it'd be great to get an updated patch addressing these issues before 0.10 so we can get it in that release. Mike
Mike, thanks for reviewing this... 1) Yes, the rate should be 48000. I hardcoded it to 44100 for testing purpopes (to see if that would fix the problem with esdsink having trouble playing certain WAV files (but it didn't). It should be changed to 48000. 2) AUDIODEV is a Sun specific feature that supports SunRay hardware, which has multiple terminals connected to a single server. ESD also has similar logic. Most audio programs that work on Sun have this logic to support SunRay. If you think it is useful to make more general, than this could be done I suppose. 3) I copied the "get_caps" logic from the esdsink, which also doesn't seem to be ever freeing the caps structure. How should this logic work to avoid leaking. Should a "caps" pointer be placed in the GstSunAudioSink structure and copied once in the init function and just re-used? The esdsink should probably also be fixed. 4) I open non-blocking just because esd does this in their Solaris specific code. I assumed it was necessary or they wouldn't be bothering doing it. 5) Regarding the open/reopen on unprepare and the close in reset, I just am doing this because it looked like it was what osssink did. I can remove these calls, but am unsure how to test and verify that it is working. If I remove these calls and just test it with a few pipes that play OGG/WAV/AU/etc. files and it seems to work, then do I know it is working or do I need to do something more to test this?
1. Ok, no problem, easy change then. 2. If this is a standard feature, fine. 3. You're leaking pad_template, not the pad itself. I think you just need to do gst_object_unref (pad_template) after you're done using it. 4. Well, either the non-blocking open is unneccesary, or it's neccesary but there's a race condition. But this doesn't need to block the plugin going in to CVS, since it's a tricky issue. 5. My reading of osssink suggests it doesn't close in reset(). In fact, it does nothing. osssink does close/reopen in unprepare, but I don't think that's needed on SunAudio, it's a quirk of the OSS interface. Also, I'd assume it'd open another close/reopen race, see 4. All of these look like they should be trivial to do and give a quick test (if not test exhaustively - but heavier testing can go on after it goes into cvs). Would you be able to do that, and file an updated patch? If you manage to do it today (and I'll understand if you can't), we can probably get it into 0.9.6, otherwise it'll go into the following (final?) 0.9.x release.
Created attachment 55119 [details] [review] updated patch for sink Here is an updated patch for the new sys/sunaudio/gstsunaudiosink.c file with the comments you suggested. I tested this code and it works just as good. So I think we should go with this. The other files needed for adding sunaudio are in the other patch, so can you just replace the gstsunaudiosink.c in the first patch with the one in the second patch? Thanks
Brian, I was going to add this plugin yesterday, but was reminded that we now have a checklist for plugins in -good, in gstreamer/docs/random/moving-plugins. Plugins are supposed to do everything in there before we put them into -good. If you really want, we can put this stuff in -bad first, but the painfulness of moving things between cvs modules is pretty high to begin with, so since this is close to being ready to go into -good, I think it'd be better to just get it done. Things from the list that it doesn't do: - use GST_(DEBUG/*)_OBJECT (conversion from just using GST_(DEBUG/*)) - documentation. There is none. - tests. I don't think that's strictly neccesary here, since it's a straightforward audio sink, automated tests aren't possible since it requires hardware access, and manual tests are easy with gst-launch lines, covered under the documentation item.
What's involved with converting GST_DEBUG to GST_DEBUG_OBJECT? Looking at the oss code it just seems it takes an additional argument of the object (e.g. the GstSunAudioSink structure) as an additional first argument. Is this all this is needed? What sort of documentation is needed? I think the code has reasonable comments in the code. Looking in other plugin directories in good, I don't see any specific documentation files anywhere, or am I missing something?
Yes, that's all that's needed for converting to GST_DEBUG_OBJECT. The checklist I pointed you at has details on what sort of docs are expected - briefly: inline gtk-doc - the docs are generated from this at build time. Not everything in plugins-good has good documentation at the moment; some of the plugins in there will probably be demoted to -bad as a result of this and other problems.
Okay, I converted the code to use GST_DEBUG_OBJECT and it seems to still work fine. I started adding comments, but need a bit more clarification since it isn't clear to me what you want. Sorry. I'm assuming that I do not need to write gtk-doc style docs for functions that are just implementing interfaces defined in various base classes. For example, the gst_sunaudiosink_prepare, gst_sunaudiosink_unprepare, etc. These are just static functions defined in the c-file and implement interfaces that I'm guessing should be explained in the base class gtk-docs (though I notice that they are not). I'm guessing I just need to provide documentation for any new interfaces I expose in the header files. There really aren't too many of them, and they are all pretty trivial functions, including the following 3: in gstsunaudiomixer.h: gst_sunaudiomixer_new gst_sunaudiomixer_free in gstsunaudiomixertrack.h: gst_sunaudiomixer_track_new I'm assuming I don't have to provide docs for the various (*_get_type) interfaces (gst_sunaudio_mixer_element_get_type in gstsunaudiomixerelement.h, gst_sunaudiomixer_track_get_type in gstsunaudiomixertrack.h, and gst_sunaudiosink_get_type in gstsunaudiosink.h. I think it is normal convention to not document get_type functions. Though I can document them if you want. Is this correct, that you just want me to provide gtk-docs for the 2 *_new() and 1 *_free() functions listed above, or do you want more?
Brian - compare with other elements, and read gstreamer/docs/README. You're not supposed to be documenting any internal API. You just need to document your element, explain whatever needs explaining about it, and provide a sample pipeline. You also need to make sure, as is explained in the README, that you run make update so that you can add your newly generated plugin file. The interface does not need to be documented - because it is an interface, the interface itself needs to be documented, not implementations of it. Otherwise it wouldn't be an interface :) Let me know if anything's unclear.
Created attachment 56413 [details] [review] upated patch with fixes and docs Here is an updated patch that works with 0.10. I had to make a few modifications to the patch to get it to work with 0.10. Like it seems the GST_PLUGIN_DEFINE requires GST_PACKAGE_NAME and GST_PACKAGE_ORIGIN instead of the older GST_PACKAGE and GST_ORIGIN. Also, I fixed two bugs that were recently reported to me. First, the set_mute in the mixer now works better since it sets the volume to zero and resets it to the right volume on unmute. The way it was working before was confusing the mixer applet and making it impossible to unmute (unless you ran another mixer program like /usr/dt/bin/sdtaudiocontrol. The other bug I fixed is now the mixer is smart enough to use AUDIODEV to build the right device name for /dev/audioctrl so it works on SunRay. Lastly, I think I updated the code and the stuff in docs/plugins so the docs should build. I wasn't able to test this because on my Solaris setup when I run "make inspect-update" I get this error: ImportError: No module named pygst It seems my system isn't set up right to build the GStreamer docs, so could someone test this for me? Sorry it took me so long to get back to this, but I have been distracted quite a bit over the past few weeks working on GDM and fixing the above bugs with this plugin.
Created attachment 56414 [details] [review] patch to fix sunaudio in 0.8 I'm not sure if there will be another 0.8 release, but this patch fixes two problems in the sunaudiomixer. These are mentioned above in the patch for 0.10, but basically it makes the mute button work right with the mixer applet by turning the volume to 0 when muted and resetting it to the real value on unmute. Also it sets the right device name on SunRay using the AUDIODEV environment variable (much like the sunaudiosink already does). If there is another 0.8 release, it would be nice to get this patch in.
Thanks Brian, Commited port to 0.10 CVS and commited fix to 0.8 branch of gst-plugins, so when the next 0.8 release is done the fix will be part of that. Please checkout gst-plugins-good from CVS and verify everything is ok. I am only able to verifty that the plugin doesn't break the build under Linux. Also the docs didn't seem to build for me, but I am not sure how that is supposed to work.
Thanks so much for integrating this. I notice one problem now that I am testing sunaudiosink with the mp3 plugin. The buffer needs to be set to 5000000 instead of 3000000 for MP3 files to play okay. So could you change these lines in gst-plugins-good/sys/sunaudio/gstsunaudiosink.c in the function gst_sunaudiosink_init: if (buffer_time < 5000000) { g_value_set_int64 (&gvalue, 5000000); g_object_set_property (G_OBJECT (sunaudiosink), "buffer-time", &gvalue); } Notice the two lines that say "5000000". Previously they said "3000000". Just changing this number makes the plugin work much better with MP3 files. Can this change be made in CVS head?
This last issue has been resolved as part of bug #327765 now if I'm not mistaken, so closing this.