[gstreamer-bugs] [Bug 152864] [PATCH] GstAlsaMixer doesn't support signals

[GStreamer (bugzilla.gnome.org)]

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




------- Comment #24 from Marc-Andre Lureau  2007-05-15 14:36 UTC -------
(In reply to comment #20)
> >+  gint			n_poll_fds;
> >+  GPollFD *		poll_fds;
> No tabs please :)

Sorry :) The indentation of gstreamer (in general) is a bit confusing. I could
not figure out what was the rule, except some wrong auto-indent here and there
:)

> >+  return (revents & G_IO_IN);
> I think it would be better to return a proper boolean here (call me paranoid).

"Panoramix" oops, "Panaroid",... cheat I call you that way

> >+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?

Right. But actually, we could remove the checking since only this file
implementation is using the gsource - with this default callback.. It's like
checking that a hard-coded string is non-null.. a bit insane, no? 


> 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.

Right! more debug stuffs!

> >+  count = snd_mixer_poll_descriptors_count (mixer->handle);
> Call me paranoid, but I think it would be good to handle count <= 0 here, since

"Panasonics"

> 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.

yeah, stack use just let me avoid the exception handling for this pointer
Looking at alsa code, it is checking the value.
ALSA simply abort (assert) if NULL is passed

> g_new0() will never return NULL unless count is 0, so we should IMHO just check
> for count == 0 directly, see above.

Cool! exception handling is boring in C. I did not know that glib ensure !=
NULL.

> 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.

Frankly, I thought quickly about that and read the GstBus code too before I
made the patch, but that's something that you have to decide. I am not sure
neither.

> Re-ordering structure members breaks ABI, don't think we can do that. Add a
> 'FIXME 0.11:' comment or so :)

Ok, sorry. Who could use structure function pointer directly? inherited objects
that would have not been recompiled? hmm.. is there any GstMixer inherited
interface?

> 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.

Nobody is sending notifications (ie using those functions) - IIRC think there
is only three implementations OSS/ALSA and?... it seems that my patch is the
first time those functions are used. So I thought it would be possible to
change to const.


-- 
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.