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 152864 - [PATCH] GstAlsaMixer doesn't support signals
[PATCH] GstAlsaMixer doesn't support signals
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal normal
: 0.10.14
Assigned To: Marc-Andre Lureau
GStreamer Maintainers
Depends on:
Blocks: 356586 370937
 
 
Reported: 2004-09-16 22:53 UTC by Jan Arne Petersen
Modified: 2009-01-25 16:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch which adds signal support to GstAlsaMixer. (13.85 KB, patch)
2004-09-16 22:55 UTC, Jan Arne Petersen
none Details | Review
a cleaner attempt (19.64 KB, patch)
2007-05-12 17:21 UTC, Marc-Andre Lureau
none Details | Review
Notify when alsa ctl change (19.72 KB, patch)
2007-05-12 19:49 UTC, Marc-Andre Lureau
committed Details | Review
use flags to describe mixer capabilities instead of type (3.13 KB, patch)
2007-05-12 20:50 UTC, Marc-Andre Lureau
needs-work Details | Review
revert back some diff in ABI (2.98 KB, patch)
2007-05-18 12:16 UTC, Marc-Andre Lureau
committed Details | Review
Remove deprecated signals from GstMixer interface (5.46 KB, patch)
2007-05-22 20:27 UTC, Marc-Andre Lureau
none Details | Review
Remove signals in mixeroptions and mixertrack (4.79 KB, patch)
2007-05-22 20:28 UTC, Marc-Andre Lureau
none Details | Review
Send GstMessage to notify mixer updates (3.07 KB, patch)
2007-05-22 20:30 UTC, Marc-Andre Lureau
none Details | Review
Monitor ALSA fds with a GstTask (12.10 KB, patch)
2007-05-23 09:40 UTC, Marc-Andre Lureau
none Details | Review
Simple example (could be eventually converted to a test later) (1.16 KB, text/x-csrc)
2007-05-23 09:45 UTC, Marc-Andre Lureau
  Details
Send GstMessage to notify mixer updates (3.50 KB, patch)
2007-05-23 12:44 UTC, Marc-Andre Lureau
none Details | Review
Monitor ALSA fds with a GstTask (12.88 KB, patch)
2007-05-23 12:46 UTC, Marc-Andre Lureau
none Details | Review
full patch on top of CVS HEAD (24.80 KB, patch)
2007-06-19 17:52 UTC, Marc-Andre Lureau
none Details | Review
Updated patch (49.70 KB, patch)
2007-07-20 16:07 UTC, Jan Schmidt
none Details | Review
test program (1.24 KB, text/x-python)
2007-07-20 17:20 UTC, Jan Schmidt
  Details

Description Jan Arne Petersen 2004-09-16 22:53:35 UTC
GstAlsaMixer never emits the 'gst_mixer_mute_toggled',
'gst_mixer_record_toggled', 'gst_mixer_volume_changed' and
'gst_mixer_option_changed' signals which are defined in GstMixer.
Comment 1 Jan Arne Petersen 2004-09-16 22:55:50 UTC
Created attachment 31632 [details] [review]
Patch which adds signal support to GstAlsaMixer.
Comment 2 Ronald Bultje 2004-09-16 23:07:22 UTC
See http://bugzilla.gnome.org/show_bug.cgi?id=94726#c11 - I expected ALSA to
handle those differently, and the signals became not-all-that-useful when I
found out I had to call into ALSA context manually to make events being
generated. That makes the whole signal stuff an over-complicated way of solving
the issue that the signals were supposed to solve...

Note that the OSS or Solaris mixer don't use the signals either.
Comment 3 Ronald Bultje 2004-09-17 06:25:17 UTC
I guess on second thought I should just implement this in OSS as well...

What do others think?
Comment 4 Christian Fredrik Kalager Schaller 2004-12-12 12:11:08 UTC
What is your current thinking on this Ronald?
Comment 5 Andy Wingo 2005-07-15 10:09:09 UTC
Signals seem to be a bad idea, judging from g-v-c's experience. Any objections
to removing them from the 0.9 mixer interface?
Comment 6 Andy Wingo 2006-01-27 16:37:56 UTC
Still not sure what to do with this. 
Comment 7 Michael Sheldon 2006-07-03 15:09:57 UTC
For Jokosher we're currently having to talk to alsa directly for handling Mixer details due to this bug. What we wish to do is monitor record-toggled to see if any channels change their record status when other channels are switched to record (so we can see if the sound card supports multiple simultanious inputs). The way we're currently solving this is to query the status of all the recording channels before and after switching a new channel's record status. As far as I'm concerned this is a perfectly acceptable way of handling it, but it's not possible through the gstreamer mixer interface since there is no provision for querying the status of channels (i.e. a gst_mixer_get_record method).
Comment 8 Andy Wingo 2006-07-21 11:40:31 UTC
(In reply to comment #7)

Michael, we do support getting the record status on a track. ALSA does not provide an interface for us to efficiently do notification signalling, so we require apps to poll if they need to, as you do with pyalsaaudio. Here is an alternate implementation of GetRecordingMixers that uses GStreamer:

import gst, gst.interfaces

def GetRecordingMixers(device):
	"""Returns a list containing all the channels which have recording switched on."""
	
	recmixers = []
	alsamixer = gst.element_factory_make('alsamixer')
        alsamixer.set_property('device', device)
        alsamixer.set_state(gst.STATE_PAUSED)

        if alsamixer.implements_interface(gst.interfaces.Mixer):
                recmixers = [(alsamixer, track) for track in alsamixer.list_tracks()
                             if (track.flags & gst.interfaces.MIXER_TRACK_INPUT
                                 and track.flags & gst.interfaces.MIXER_TRACK_RECORD
                                 # Ignore 'Capture' channel due to it being a
                                 # requirement for recording on most low-end
                                 # cards
                                 and track.label != 'Capture')]
        else:
                print ('Could not get the mixer for ALSA device %s, '
                       'check your permissions' % (device,))
                recmixers = []

	print device, recmixers
	return recmixers
Comment 9 Michael Sheldon 2006-07-21 12:57:56 UTC
Thanks Andy, I didn't notice the MixerTrack flags. I've commited you patch (after a bit of editing) and all is well in Jokosherland. So as we're concerned the Mixer  and MixerTrack interfaces supply everything necessary without the need for signals.
Comment 10 Tim-Philipp Müller 2006-07-23 11:18:59 UTC
So, what to do with this one then?

 - WONTFIX: everything can be done via polling, document
   signals as non-functional in docs and remove them in 0.11

 - add generic GstMixerTrack helper source/setup function that
   hooks into a GLib main context/loop and does polling in
   intervals and emits signals based on that. This would
   primarily be for code-sharing purposes and would
   work for all mixer implementations immediately.

 - do clever stuff in the mixer implementation, like alsa
   (but: does not really work like we want to if I understand
   Ronald correctly; requires either a running main context or a
   thread with our own main context/main loop if I understand
   correctly (?)).

 - any other option I forgot?
Comment 11 Andy Wingo 2006-07-24 08:46:59 UTC
OK I just spent about an hour poking in source code. It seems pulseaudio is able to get the notifications:

https://tango.0pointer.de/pipermail/pulseaudio-commits/2006-February/000748.html

http://pulseaudio.org/browser/trunk/src/modules/module-alsa-sink.c#L167

So... we should just figure out how these additional fd's integrate with gst.
Comment 12 Matthew Garrett 2006-09-05 11:55:34 UTC
Right now, the gnome mixer is polling for volume changes 10 times a second. I think avoiding that would be nice if possible.
Comment 13 Matthias Clasen 2006-12-05 04:30:54 UTC
Any progress on this ? 
On my desktop, the mixer applet is currently the prime offender when it comes to context switching (apart from at-spi-registryd).
Comment 14 Ronald Bultje 2007-01-10 14:04:40 UTC
You cannot just close this, you absolutely want mainloop integration and notification, even if only for saving battery power and being a greenpeace hippie. So for all of those, you need mainloop registration as a very first thing. For alsa, I suppose it's snd_mixer_poll_descriptors() or so, but you'd need to look at the docs. Pulse same thing, you need the fd to be notified. I would personally go for a mainloop hook, but if you want to be really cool, a thread would do also (and would provide more instant update notifications). I don't know details about mainloop integration exactly, I looked at it at one point and forgot.

Someone in the gst camp needs to sit on this for a few days and do it. Write a testcase, possibly use the mixer as testcase and even provide patches for the mixer-applet (yay!), and just make it work. The mixer applet (and gnome-volume-control, eventually) would be a lot cleaner with this.

For the non-workers (oss, probably sunaudio, ...), you can do 2 things. Either set a flag NO_SIGNALS on the mixer, because it can't do it without basically querying everything, or (and?) write a helper function that the app can call every once in a while that emits those signals automatically. In the mixer applet and such, we should document that some of them are not actively updated and use polling and that it's bad for your battery and will kill us all or something. The second approach would be nicer.

When the applet is done, don't forget that gnome-volume-control has the exact same code, but I guess the applet is currently more important because every fedora, ubuntu etc. desktop ships with it. If you make a mixer-applet patch, I'm sure I can come up with something for gnome-volume-control.
Comment 15 Marc-Andre Lureau 2007-05-12 17:21:20 UTC
Created attachment 88089 [details] [review]
a cleaner attempt

for the greenpeace hippies!
Comment 16 Marc-Andre Lureau 2007-05-12 19:49:23 UTC
Created attachment 88094 [details] [review]
Notify when alsa ctl change

Previous patch was doing  

if (!revents)
    revents = G_IO_IN;

(somewhere alsa/utils was doing this)  

Simple case was working with it though.
This snippet seems unnecessary, and it looks obviously wrong (in a glib ctxt).
Comment 17 Marc-Andre Lureau 2007-05-12 20:50:08 UTC
Created attachment 88097 [details] [review]
use flags to describe mixer capabilities instead of type

use flags to describe mixer capabilities instead of type,

to be discussed, as suggested Ronald.

Quick patch will be necessary in good/ugly/bad too.
Read-only compatibility macro is there though.
Comment 18 Ronald Bultje 2007-05-12 20:56:58 UTC
Cool stuff! I'm not the right person to review this, I guess, but I like the fact that you're working on this. :).
Comment 19 Marc-Andre Lureau 2007-05-12 22:35:42 UTC
(In reply to comment #18)
> Cool stuff! I'm not the right person to review this, I guess, but I like the
> fact that you're working on this. :).
> 

Hehe!

+I just made a patch for the applet!
Comment 20 Tim-Philipp Müller 2007-05-15 13:24:22 UTC
Comment on attachment 88094 [details] [review]
Notify when alsa ctl change

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.
Comment 21 Tim-Philipp Müller 2007-05-15 13:29:55 UTC
Comment on attachment 88097 [details] [review]
use flags to describe mixer capabilities instead of type

>--- a/gst-libs/gst/interfaces/mixer.h
>+++ b/gst-libs/gst/interfaces/mixer.h
> 
>-#define GST_MIXER_TYPE(klass) (klass->mixer_type)
>+/* compatibility with version <= 0.10.12 */
>+#define GST_MIXER_TYPE(klass)  (GST_MIXER_IS_HARDWARE(klass) ? GST_MIXER_HARDWARE : GST_MIXER_SOFTWARE)
>+#define GST_MIXER_FLAGS(klass) (klass->flags)

This only maintains API, but not ABI, if I'm not mistaken.

> struct _GstMixerClass {
>   GTypeInterface klass;
> 
>-  GstMixerType mixer_type;
>+  GstMixerFlags flags;

This breaks ABI, I think.

I don't know if we actually need this new flag. I guess it depends on how we solve the 'big picture' problem. If we added a new vfunc to return a monitoring GSource then we could just add API to check if a particular mixer class provides this or not. I'll try to solicit some more comments from other people.
Comment 22 Andy Wingo 2007-05-15 13:44:17 UTC
I think you need to add a lock in GstAlsaMixer, and access members of the AlsaMixer structure only when holding the lock. Looks like you might do that best by making it a GstObject.
Comment 23 Marc-Andre Lureau 2007-05-15 14:14:26 UTC
(In reply to comment #22)
> I think you need to add a lock in GstAlsaMixer, and access members of the
> AlsaMixer structure only when holding the lock. Looks like you might do that
> best by making it a GstObject.
> 

Can GstAlsaMixerElement be created/used in a different thread (context) than what  g_main_context_default() return?

Does GstAlsaMixerElement already provide locking?

Is that critical? 
Comment 24 Marc-Andre Lureau 2007-05-15 14:36:48 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.
Comment 25 Marc-Andre Lureau 2007-05-15 14:58:19 UTC
Btw, thanks for the comments :)

Before we get the comments, Stefan applied the patch with minor modifications.
I am working on a new patch to fix stuffs in the comments.(In reply to comment #21)

(In reply to comment #21)
> If we added a new vfunc to return a monitoring
> GSource then we could just add API to check if a particular mixer class
> provides this or not. I'll try to solicit some more comments from other people.

Except that we could not ensure that this source will be poll'ed in the same context/thread than alsa - if I understand the comments of Ronald correctly and look at how the alsa-plugins works.

Basically, alsa-plugins returns fds. That doesn't tell you were it comes from, but at least I am quite conviced that those fds should be handled in the same thread as alsa API is called. Though I barely know about this kind of threading details...

Comment 26 Stefan Sauer (gstreamer, gtkdoc dev) 2007-05-15 15:34:29 UTC
A cleaned up version that addresses the issues Tim raised abound the alsa changes has been commited. the interface break still needs to be reverted.
Comment 27 Marc-Andre Lureau 2007-05-18 12:16:09 UTC
Created attachment 88386 [details] [review]
revert back some diff in ABI
Comment 28 Marc-Andre Lureau 2007-05-18 12:20:01 UTC
Should we close this bug and reopen a new bug for the remaining questions/issues once the latest patch is applied:

 - locking in GstAlsaMixer (nobody seems to clearly understand if its necessary)

 - mixer API changes (GSource, flags)
Comment 29 Tim-Philipp Müller 2007-05-18 14:48:23 UTC
> Should we close this bug and reopen a new bug for the remaining
> questions/issues once the latest patch is applied:

IMHO this stuff is currently not releasable yet, so I am not sure what the point of opening another bug for it would be.


>  - locking in GstAlsaMixer (nobody seems to clearly understand if its
> necessary)

Locking is necessary, as is proper refcounting, to make sure objects are alive when callbacks are executed. Currently it's possible to free a mixer object in one thread while its GSource callback is executed in another thread, if I'm not mistaken. Admittedly this isn't a big problem for the mixer applet or gnome-volume-control, but it should be handled correctly, otherwise we get to a point where we can't fix it properly without breaking things later.


>  - mixer API changes (GSource, flags) 

It looks to me like at least the first is directly related to the above.


[From earlier comments:]

> > I wonder whether we should just not add the GSource automatically, (...)
> 
> 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.

Asking around on IRC, I got opinions along the line of 'setting up the GSource automagically is probably not a good idea' and also that the vfunc-returning-a-GSource thing I suggested above might be overdoing it a bit, so no clear decision here yet.

Three solutions I can think of right now:

 (a) start a monitoring thread in NULL => READY state change with its
     own main context, and shut that down again in READY => NULL. This
     would make things work similar to other elements.

 (b) require the application to explicitly start/stop monitoring

 (c) just document the limitations/problems and leave it at that
     (can't say I like this very much though).


> > [const-ifications]
> 
> 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.

Ah, I missed the fact that those aren't used by any of our implementations (sunaudio is the third btw). I guess that's fine then, it doesn't really break anything after all. Sorry for the noise.


I'll try to get some more opinions on how to solve this, so it gets into the upcoming release. Maybe I'm just overcomplicating things, who knows :)
Comment 30 Stefan Sauer (gstreamer, gtkdoc dev) 2007-05-18 15:12:58 UTC
2007-05-18  Stefan Kost  <ensonic@users.sf.net>

        patch by: Marc-Andre Lureau <marcandre.lureau@gmail.com>

        * gst-libs/gst/interfaces/mixer.h (mixer_type, option_changed,
          set_option, get_option, _gst_reserved):
          Revert reordering functions (keep ABI).


I keept the const there.
Comment 31 Marc-Andre Lureau 2007-05-22 14:03:09 UTC
To keep this bug active, hehe :) I recently learned that the set of file descriptors that ALSA return might change over time (this is not documented). The current code does not handle that. I am on it.
Comment 32 Marc-Andre Lureau 2007-05-22 20:27:33 UTC
Created attachment 88634 [details] [review]
Remove deprecated signals from GstMixer interface

Remove deprecated signals from GstMixer interface

Keeping the default vtable as such, marking signal handlers as deprecated.
Triggering functions are empty, they will change to send GstMessage instead of firing signals.
---
 gst-libs/gst/interfaces/mixer.c |   62 ---------------------------------------
 gst-libs/gst/interfaces/mixer.h |    6 ++-
 2 files changed, 4 insertions(+), 64 deletions(-)
Comment 33 Marc-Andre Lureau 2007-05-22 20:28:18 UTC
Created attachment 88635 [details] [review]
Remove signals in mixeroptions and mixertrack

Deprecated symbols in ABI kept and mark as such.
---
 gst-libs/gst/interfaces/mixeroptions.c |   15 ---------------
 gst-libs/gst/interfaces/mixeroptions.h |    2 ++
 gst-libs/gst/interfaces/mixertrack.c   |   32 --------------------------------
 gst-libs/gst/interfaces/mixertrack.h   |    4 +++-
 4 files changed, 5 insertions(+), 48 deletions(-)
Comment 34 Marc-Andre Lureau 2007-05-22 20:30:53 UTC
Created attachment 88636 [details] [review]
Send GstMessage to notify mixer updates

Comments welcome - it has still to be used and tested!

Notes: track and trackoptions children are both passed with struct field "track"
 GValue
Children do not notify anymore (since they do not inherit from GstElement). I pe
rsonnaly think it was not really necessary anyway.
---
 gst-libs/gst/interfaces/mixer.c |   55 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 55 insertions(+), 0 deletions(-)
Comment 35 Marc-Andre Lureau 2007-05-23 09:40:54 UTC
Created attachment 88652 [details] [review]
Monitor ALSA fds with a GstTask

With this set of 4 patches, I got crashes in message creation. Something I misused, most probably with GstStructure:

  • #6 gst_structure_new
    at gststructure.c line 179
  • #7 gst_mixer_volume_changed
    at mixer.c line 330


To poll ALSA fds, I decided to drop the custom GSource and use poll directly inside the task.
I am not confident how to handle errors in the task.
I don't know if RecMutex are necessary (it depends if ALSA plugins could call cb from different threads - ideally not)
I am unsure about the (un)locking places (I am not an expert in MT environnement)

To sum up... this is a draft patch :)
---
 ext/alsa/gstalsamixer.c |  261 +++++++++++++++++++----------------------------
 ext/alsa/gstalsamixer.h |    5 +-
 2 files changed, 108 insertions(+), 158 deletions(-)
Comment 36 Marc-Andre Lureau 2007-05-23 09:45:20 UTC
Created attachment 88654 [details]
Simple example (could be eventually converted to a test later)
Comment 37 Marc-Andre Lureau 2007-05-23 12:44:19 UTC
Created attachment 88663 [details] [review]
Send GstMessage to notify mixer updates

Notes: track and trackoptions children are both passed with struct field "track" GValue
Children do not notify anymore (since they do not inherit from GstElement). I personnaly think it was not really necessary anyway.

Updated: fix gvalue list
TODO: explain why I see the messages going in the bus with debug log, but I cannot catch them
Comment 38 Marc-Andre Lureau 2007-05-23 12:46:03 UTC
Created attachment 88664 [details] [review]
Monitor ALSA fds with a GstTask


To poll ALSA fds, I decided to drop the custom GSource and use poll directly inside the task.
I am not confident how to handle errors in the task.
I don't know if RecMutex are necessary (it depends if ALSA plugins could call cb from different threads - ideally not)
I am unsure about the (un)locking places (I am not an expert in MT environnement)

Updated: kill task with a pipe, free locks after the task is really finished
Comment 39 Jan Schmidt 2007-05-25 10:03:28 UTC
Given that this bug still seems to need work, and I want to make a pre-release today, I'm going to revert the commits that have gone in so far and release with the same code as 0.10.12 plugins base.

We'll be making another release of gst-plugins-base before Gnome 2.20, so we have until then to get this ready and committed in final form.

I'll be reverting these:

cvs diff -r1.20 -rHEAD gst-libs/gst/interfaces/mixer.c
cvs diff -r1.17 -rHEAD gst-libs/gst/interfaces/mixer.h
cvs diff -r1.34 -rHEAD ext/alsa/gstalsamixer.c
cvs diff -r1.11 -rHEAD ext/alsa/gstalsamixer.h
cvs diff -r1.10 -rHEAD ext/alsa/gstalsamixerelement.c
cvs diff -r1.14 -rHEAD ext/alsa/gstalsamixertrack.c
Comment 40 Marc-Andre Lureau 2007-06-01 18:05:41 UTC

(In reply to comment #39) 
> I'll be reverting these:

Hi, I guess the 4 patches I provided probably don't apply easily then..

They were ready for review, in case you have 10 minutes anyway :)

attachment 88634 [details] [review] & attachment 88635 [details] [review] just remove gstmixer gsignals
attachment 88663 [details] [review] to send gstmsg to notify mixer updates
attachment 88664 [details] [review] start a gsttask to poll alsa filedescriptors

I can provide a big-patch that applied on top of current HEAD if necessary. It would be nice to have some feedback on the previous patch though. Thanks!
Comment 41 Tim-Philipp Müller 2007-06-01 18:12:49 UTC
> I can provide a big-patch that applied on top of current HEAD if necessary.

If it's not too much work, it would be great if you could attach that as well. I think it might be easier to review against the 'original' state without any polling changes.
 
Comment 42 Jan Schmidt 2007-06-18 10:21:04 UTC
Ping? We need to get moving on this bug if we intend to have it merged, tested and stable before Gnome 2.20
Comment 43 Marc-Andre Lureau 2007-06-19 17:52:10 UTC
Created attachment 90283 [details] [review]
full patch on top of CVS HEAD 

the log shows the gstmsg going on the bus, 
(I don't know how to write a working client)
Comment 44 Jan Schmidt 2007-07-20 16:07:40 UTC
Created attachment 92054 [details] [review]
Updated patch

This is an updated patch. It changes:
* Deprecate the signals, but still add them to the object so that any programs which connect to the signals don't signal a warning
* Adds some API to recognise and parse the notification messages on the bus
* Adds docs and a test for the new API
* Adds a get_flags function to retrieve flags from a mixer implementation, similar to the GstMixerTrack/Options, allowing apps to detect whether they need to poll for changes or not.
* Makes alsamixer check whether mute/record/volumes have changed individually instead of sending 3 messages at once regardless of whether one of the 3 has changed or not.

At this stage, I consider this ready to commit if people are happy with the new API.
Comment 45 Jan Schmidt 2007-07-20 17:20:12 UTC
Created attachment 92058 [details]
test program

Here's a little python test program that prints out the bus messages as they are received. Ignore the fact that it prints the GstMixerTrack as NULL - that's just because there's no serialisation function for a GstMixerTrack - the pointer in the message is fine.
Comment 46 Jan Schmidt 2007-07-21 09:29:47 UTC
I committed the current version to CVS so people can play with it. If people don't like something, we'll re-open.

2007-07-20  Jan Schmidt  <thaytan@noraisin.net>

        * docs/libs/gst-plugins-base-libs-sections.txt:
        * ext/alsa/gstalsamixer.c:
        * ext/alsa/gstalsamixer.h:
        * ext/alsa/gstalsamixerelement.c:
        * ext/alsa/gstalsamixertrack.c:
        * gst-libs/gst/interfaces/mixer.c:
        * gst-libs/gst/interfaces/mixer.h:
        * gst-libs/gst/interfaces/mixeroptions.c:
        * gst-libs/gst/interfaces/mixeroptions.h:
        * gst-libs/gst/interfaces/mixertrack.c:
        * gst-libs/gst/interfaces/mixertrack.h:
        * tests/check/Makefile.am:
        * tests/check/libs/mixer.c:

        Patch By: Marc-Andre Lureau <marcandre.lureau@gmail.com>
        Fixes: #152864

        Add support for notifying mixer changes on the message bus, and
        implement it in alsamixer.

        API: gst_mixer_get_mixer_flags
        API: gst_mixer_message_parse_mute_toggled
        API: gst_mixer_message_parse_record_toggled
        API: gst_mixer_message_parse_volume_changed
        API: gst_mixer_message_parse_option_changed
        API: GstMixerMessageType
        API: GstMixerFlags