After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 321269 - [sunaudio] port to GStreamer-0.10
[sunaudio] port to GStreamer-0.10
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other opensolaris
: Normal normal
: 0.10.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Reported: 2005-11-11 22:47 UTC by Brian Cameron
Modified: 2006-01-23 11:22 UTC
See Also:
GNOME target: ---
GNOME version: ---

patch to add sunaudio mixer and sink plugins. (43.28 KB, patch)
2005-11-11 22:48 UTC, Brian Cameron
none Details | Review
updated patch for sink (11.88 KB, patch)
2005-11-23 01:15 UTC, Brian Cameron
none Details | Review
upated patch with fixes and docs (46.27 KB, patch)
2005-12-27 02:16 UTC, Brian Cameron
committed Details | Review
patch to fix sunaudio in 0.8 (3.39 KB, patch)
2005-12-27 02:18 UTC, Brian Cameron
committed Details | Review

Description Brian Cameron 2005-11-11 22:47:05 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.
Comment 1 Brian Cameron 2005-11-11 22:48:42 UTC
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.
Comment 2 Brian Cameron 2005-11-11 22:49:02 UTC
Note, the sink plugin has been tested and works.
Comment 3 Michael Smith 2005-11-21 14:46:56 UTC
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

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.


Comment 4 Brian Cameron 2005-11-21 20:40:58 UTC
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

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?
Comment 5 Michael Smith 2005-11-22 08:59:49 UTC
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.
Comment 6 Brian Cameron 2005-11-23 01:15:06 UTC
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?

Comment 7 Michael Smith 2005-11-24 11:57:39 UTC

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:
   (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.
Comment 8 Brian Cameron 2005-11-24 21:11:53 UTC
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

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?
Comment 9 Michael Smith 2005-11-24 22:18:04 UTC
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
Comment 10 Brian Cameron 2005-11-25 21:07:31 UTC
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:


in gstsunaudiomixertrack.h:


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?

Comment 11 Thomas Vander Stichele 2005-11-28 12:16:10 UTC
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.
Comment 12 Brian Cameron 2005-12-27 02:16:28 UTC
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.
Comment 13 Brian Cameron 2005-12-27 02:18:11 UTC
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.
Comment 14 Christian Fredrik Kalager Schaller 2006-01-09 17:06:57 UTC
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.

Comment 15 Brian Cameron 2006-01-13 18:30:40 UTC
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?

Comment 16 Tim-Philipp Müller 2006-01-23 11:22:12 UTC
This last issue has been resolved as part of bug #327765 now if I'm not mistaken, so closing this.