Created attachment 94978 [details] [review]
Mixer interface patch for playbin

I believe the current setup is intentional and the desired use case for most desktop applications, as changing the mixer setting (and thus the volume of all applications usually) is regarded to be confusing for the user.

Other use cases could probably be catered for if, but IMHO that should be done by adding additional API instead of changing the behaviour of the current API.


I also see some practical issues with this patch, some of which stem from the awkward GstImplementsInterface API that GstMixer implements:

 - setting the same volume for each track doesn't really seem
   right, but in any case you probably want to skip non-playback
   tracks and also take into account track->min_volume. I am not
   sure if setting the volume on all mixer tracks would be an
   acceptable thing to do even for new API - we'd need a smarter
   solution for that, I think.

 - one would probably need to keep track of changes in the mixer
   volume too, so that get_volume() on playbin returns the actual
   value of the mixer volume control, and not just value last set
   via playbin (which the user might have changed through another
   application since then)

 - keep in mind that whether the mixer interface is implemented
   by an element depends on its state too (the device must be
   open for this); this means if the application tries to change
   the volume while the mixer is in the 'wrong' state, you'll
   modify the setting of the 'volume' element instead, which
   will persist when the sink changes state again later and
   lead to an aggregated volume that's too low, if I'm not
   mistaken.

Tim, when one uses alsa via dmix, esound or pulseaudio then changing the volume on themixer should only affect the current stream, right. And if one uses alsa directly or oss then affecting the mixer is okay, as there cannot be nother stream at the same time.

This seems to be slightly related to Bug 302798.

Many OSS and ALSA drivers allow hardware mixing, i.e. having the device opened more than once at the same time and playing multiple streams. So affecting the mixer for OSS/ALSA has exactly the same problems.

Can it be detected wheter the mixer acts globaly or on the stream?

No idea, sorry

(In reply to comment #4)
> Many OSS and ALSA drivers allow hardware mixing, i.e. having the device opened
> more than once at the same time and playing multiple streams. 

I thought too, but this is not correct. If it works for you, it's usually, because thanks to dmix.

(In reply to comment #5)
> Can it be detected wheter the mixer acts globaly or on the stream?

I am quite sure it will be possible to ask pulseaudio. 

For hw mixing in pulse, follow:
http://www.pulseaudio.org/ticket/25

(reminder, I still have stuff to say here :)

(In reply to comment #7)
> (In reply to comment #4)
> > Many OSS and ALSA drivers allow hardware mixing, i.e. having the device opened
> > more than once at the same time and playing multiple streams. 
> 
> I thought too, but this is not correct. If it works for you, it's usually,
> because thanks to dmix.

Definitely not... that was with my old card in times where dmix didn't even exist... and it worked with the OSS drivers too :)

Tim mentions that this could be a (hacky) way of solving the problem.

None of the audio sinks currently implement the mixer interface. If we said that only sinks capable of per-stream volume implement this mixer interface (or flag it with a specific property), then this change would allow applications to use that per-stream volume instead of a volume within the pipeline.

The patch is missing property notifications when the mixer volume is changed outside playbin.

This means that changing the per-stream volume in pavucontrol wouldn't change the volume in the application's UI.

What about sinks that do not have per-stream volume control? The application should use it's own in this case, right?

With the current design, only PulseAudio will allow remote volume control.

What about manipulating application volume without PulseAudio? I would suggest an interface for that kind of remote control, but I can hear you saying "to a dbus guy, everything is a dbus pb"...:)

(In reply to comment #11)
> What about sinks that do not have per-stream volume control? The application
> should use it's own in this case, right?

Yes.

> With the current design, only PulseAudio will allow remote volume control.

Remote control volume? What does that mean?

> What about manipulating application volume without PulseAudio? I would suggest
> an interface for that kind of remote control, but I can hear you saying "to a
> dbus guy, everything is a dbus pb"...:)

That doesn't seem to have anything to do with the problem we're discussing in this bug, and I don't see how changing the volume in applications has anything to do with which mixer playbin would be using.

I'm thinking about moving the volume control in the baseaudiosink class where it can be changed with a lower latency. Optionally sinks that have other ways of changing the per-stream volume could override and do something else (pulseaudio, ...). 

The end result would be that every audio sink has a volume (interface? property?) that changes the per-stream volume.

(In reply to comment #12)
> Remote control volume? What does that mean?

Changing the volume from an external process.
 
> > What about manipulating application volume without PulseAudio? I would suggest
> > an interface for that kind of remote control, but I can hear you saying "to a
> > dbus guy, everything is a dbus pb"...:)
> 
> That doesn't seem to have anything to do with the problem we're discussing in
> this bug, and I don't see how changing the volume in applications has anything
> to do with which mixer playbin would be using.

It's related since pavucontrol will be able to change the volume of pulseaudio sinks. You reviewed the patch making the same kind of comment: it goes one way (from the app/playbin), what about the other way (from pavucontrol/remote)

(In reply to comment #14)
> (In reply to comment #12)
> > Remote control volume? What does that mean?
> 
> Changing the volume from an external process.
> 
> > > What about manipulating application volume without PulseAudio? I would suggest
> > > an interface for that kind of remote control, but I can hear you saying "to a
> > > dbus guy, everything is a dbus pb"...:)
> > 
> > That doesn't seem to have anything to do with the problem we're discussing in
> > this bug, and I don't see how changing the volume in applications has anything
> > to do with which mixer playbin would be using.
> 
> It's related since pavucontrol will be able to change the volume of pulseaudio
> sinks. You reviewed the patch making the same kind of comment: it goes one way
> (from the app/playbin), what about the other way (from pavucontrol/remote)

I'm talking about the mixer in playbin noticing the changes done by external programs, just like the volume applet syncs the volume with gnome-volume-control. I've never talked about any D-Bus (or whatever) API to modify the application's volume from another application. This is completely out of topic here.

(In reply to comment #13)
> I'm thinking about moving the volume control in the baseaudiosink class where
> it can be changed with a lower latency. Optionally sinks that have other ways
> of changing the per-stream volume could override and do something else
> (pulseaudio, ...). 
> 
> The end result would be that every audio sink has a volume (interface?
> property?) that changes the per-stream volume.

Sounds like it would fit exactly what we'd need.

Regarding comment #13, whatabout moving bother "volume" and "mute"?

(In reply to comment #15)
> I'm talking about the mixer in playbin noticing the changes done by external
> programs, just like the volume applet syncs the volume with
> gnome-volume-control. 

ok, I got it  

playbin2 can now set the volume property on the sink element (if it's available). Special sinks that have some way to control the volume of this particular stream can implement the property to do the right thing. If there is no sink with the property, playbin2 falls back to using the volume element.

*** Bug 486667 has been marked as a duplicate of this bug. ***

So if playbin2 can do it dow we still want to do it for playbin? Otherwise should it be closed as WONTFIX?

Marking as obsolete, implement stream volume in the sinks with a 'volume' property.