[gstreamer-bugs] [Bug 152864] [PATCH] GstAlsaMixer doesn't support signals
GStreamer (bugzilla.gnome.org)
bugzilla-daemon at bugzilla.gnome.org
Tue May 15 06:24:24 PDT 2007
If you have any questions why you received this email, please see the text at
the end of this email. Replies to this email are NOT read, please see the text
at the end of this email. You can add comments to this bug at:
http://bugzilla.gnome.org/show_bug.cgi?id=152864
GStreamer | gst-plugins-base | Ver: HEAD CVS
Tim-Philipp Müller changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #88094|none |needs-work
Flag| |
------- Comment #20 from Tim-Philipp Müller 2007-05-15 13:24 UTC -------
(From update of attachment 88094)
Cool stuff, thanks for working on this!
The patch looks good to me in general, but needs a bit more work on the
details, the big picture, and preferably a small test program that we can drop
in -base/tests/icles too.
Comments inline (mostly just nitpicking though).
The main problem is that GstMixer is an exported interface and we absolutely
need to maintain its API and ABI. This is not so much a problem with the first
patch (since you just reordered vfuncs within the struct), but your second
patch only maintains API and breaks ABI in places (I'll comment on that in a
separate comment though).
>+ GSource source;
>+ gint n_poll_fds;
>+ GPollFD * poll_fds;
No tabs please :)
>+static gboolean
>+gst_alsa_mixer_check (GSource * source)
>+{
>+ (...)
>+ return (revents & G_IO_IN);
>+}
I think it would be better to return a proper boolean here (call me paranoid).
>+static gboolean
>+gst_alsa_mixer_dispatch (GSource * source, GSourceFunc callback, gpointer user_data)
>+{
>+ if (!callback) {
>+ GST_WARNING ("Cannot get callback from ALSA mixer watcher");
>+ }
>+
>+ return (*callback) (user_data);
>+}
Isn't there a 'return FALSE' missing in the !callback case?
>+static gboolean
>+gst_alsa_mixer_handle_source_callback (gpointer data)
>+{
>+ GstAlsaMixer *mixer = (GstAlsaMixer *)data;
>+
>+ GST_WARNING ("Source cb");
>+ snd_mixer_handle_events (mixer->handle);
>+
>+ return TRUE;
>+}
This is just one of a few cases where GST_WARNING is used instead of, say,
GST_LOG or GST_DEBUG. Please change those to the appropriate level. Might be
useful to also dump the alsa element name in the alsa callbacks where we have a
snd_mixer_elem_t.
>+ count = snd_mixer_poll_descriptors_count (mixer->handle);
>+
>+ pfds = g_newa (struct pollfd, count);
>+
>+ watch->n_poll_fds = snd_mixer_poll_descriptors (mixer->handle, pfds, count);;
Call me paranoid, but I think it would be good to handle count <= 0 here, since
g_new* will usually return NULL for a count of 0 and I wouldn't count on alsa
reacting kindly to that if passed in again. Also, I'd rather use the
old-fashioned g_new0() + g_free() here, as this isn't particular
performance-sensitive code anyway.
>+ watch->poll_fds = g_new0 (GPollFD, count);
>+ if (watch->poll_fds == NULL) {
>+ GST_WARNING ("Cannot allocate poll descriptors");
>+ g_source_destroy (mixer->handle_source);
>+ mixer->handle_source = NULL;
>+ return;
>+ }
g_new0() will never return NULL unless count is 0, so we should IMHO just check
for count == 0 directly, see above.
>+ /* FIXME mixer has to be protected, since cb could be made from different thread ? */
>+ g_source_set_callback (mixer->handle_source, gst_alsa_mixer_handle_source_callback, mixer, NULL);
>+ g_source_attach (mixer->handle_source, main_context);
I do think we have to take into account threading/refcounting issues here, but
I'm not really sure how to solve this properly so that it works automatically
in all scenarios.
I wonder whether we should just not add the GSource automatically, but instead
add a vfunc of some sort to GstMixerClass (for example one that returns a
GSource), so that any application has to start and stop the
monitoring-for-changes stuff itself (using gst_mixer_*() convenience API which
we will add and which can cater for both, applications running a GLib main loop
and applications that don't). That way we could just ref and unref (and take
locks as appropriate in the callbacks). This is similar to GstBus in a way
where a signal watch has to be added explicitly.
>--- a/gst-libs/gst/interfaces/mixer.h
>+++ b/gst-libs/gst/interfaces/mixer.h
>@@ -74,6 +74,11 @@ struct _GstMixerClass {
> void (* set_record) (GstMixer *mixer,
> GstMixerTrack *track,
> gboolean record);
>+ void (* set_option) (GstMixer *mixer,
>+ GstMixerOptions *opts,
>+ gchar *value);
>+ const gchar * (* get_option) (GstMixer *mixer,
>+ GstMixerOptions *opts);
>
> /* signals */
> void (* mute_toggled) (GstMixer *mixer,
>@@ -84,17 +89,11 @@ struct _GstMixerClass {
> gboolean record);
> void (* volume_changed) (GstMixer *mixer,
> GstMixerTrack *channel,
>- gint *volumes);
>-
>- void (* set_option) (GstMixer *mixer,
>- GstMixerOptions *opts,
>- gchar *value);
>- const gchar * (* get_option) (GstMixer *mixer,
>- GstMixerOptions *opts);
>+ const gint *volumes);
>
> void (* option_changed) (GstMixer *mixer,
> GstMixerOptions *opts,
>- gchar *option);
>+ const gchar *option);
>
> /*< private >*/
> gpointer _gst_reserved[GST_PADDING];
Re-ordering structure members breaks ABI, don't think we can do that. Add a
'FIXME 0.11:' comment or so :)
I'm also not sure about the const-ifications (even if correct), since it will
cause compiler warnings for people if we change that. Dunno, personally I'd
rather just add another FIXME.
--
See http://bugzilla.gnome.org/page.cgi?id=email.html for more info about why you received
this email, why you can't respond via email, how to stop receiving
emails (or reduce the number you receive), and how to contact someone
if you are having problems with the system.
You can add comments to this bug at http://bugzilla.gnome.org/show_bug.cgi?id=152864.
More information about the Gstreamer-bugs
mailing list