[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 #29 from Tim-Philipp Müller  2007-05-18 14:48 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 :)


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