[gstreamer-bugs] [Bug 321269] add sunaudio to 0.9

[GStreamer (bugzilla.gnome.org)]

Do not reply to this via email (we are currently unable to handle email
responses and they get discarded).  You can add comments to this bug at
http://bugzilla.gnome.org/show_bug.cgi?id=321269
 GStreamer | gst-plugins-good | Ver: HEAD CVS





------- Additional Comments From Michael Smith  2005-11-21 14:46 -------
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




------- You are receiving this mail because: -------
You are the assignee for the bug.
You are the QA contact for the bug.