Bug 321288 - [API+,B+] PAStreamUUID not passed to PA in a reentrant fashion
Summary: [API+,B+] PAStreamUUID not passed to PA in a reentrant fashion
Status: RESOLVED FIXED
Alias: None
Product: Phonon
Classification: Unclassified
Component: Pulsesupport (show other bugs)
Version: 4.6.0
Platform: Gentoo Packages Linux
: NOR normal (vote)
Target Milestone: 4.7
Assignee: Harald Sitter
URL:
Keywords:
Depends on:
Blocks: 322791
  Show dependency treegraph
 
Reported: 2013-06-17 18:15 UTC by kripton
Modified: 2013-07-24 21:03 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In: 4.7.0


Attachments
volumetest_wovids.tar.xz (1.90 KB, application/x-xz)
2013-06-17 18:18 UTC, kripton
Details
phonon-two-videoplayer.log (20.05 KB, text/x-log)
2013-06-17 18:20 UTC, kripton
Details
phonon-workaround.patch (1.42 KB, patch)
2013-06-17 18:21 UTC, kripton
Details

Note You need to log in before you can comment on or make changes to this bug.
Description kripton 2013-06-17 18:15:43 UTC
as discussed in #321172:
The attached example creates to instances of Phonon::VideoPlayer, each using its own instance of Phonon::AudioOutput. If one now calls setVolume() on the VideoPlayer-objects, this only works for the latest created instance and does nothing for all other instances.

Reproducible: Always

Actual Results:  
Volume doesn't change

Expected Results:  
Volume changes
Comment 1 kripton 2013-06-17 18:18:06 UTC
Created attachment 80590 [details]
volumetest_wovids.tar.xz

Example needs two videos "test1.ogg" and "test2.ogg". Play them using the "Play" button and the two "Mute" buttons should mute them individually. The same file with two short videos can be downloaded from: http://kripton.kripserver.net/n0th1ng/volumetest.tar.xz (cannot attach here due to size constraints)
Comment 2 kripton 2013-06-17 18:20:25 UTC
Created attachment 80591 [details]
phonon-two-videoplayer.log

Phonon debug log when running the example from the tar archive and clicking the mute buttons several times
Comment 3 kripton 2013-06-17 18:21:26 UTC
Created attachment 80592 [details]
phonon-workaround.patch

Workaround I use her locally: Change the volume yourself (in the backend?) and don't rely on PulseAudio to do the volume setting. Not a fix, just a workaround.
Comment 4 kripton 2013-06-17 18:21:36 UTC
related to https://qt-project.org/forums/viewthread/27655/
Comment 5 Harald Sitter 2013-06-17 23:01:31 UTC
tldr: we use a custom PA property 'streamid' to map PA streams to our AOs, the property is set via PulseSupport when an AO is *created* by setting an environment variable, but this is only picked up when gst/vlc actually create the PA stream; so only the last created AO's streamid will be used for *all* streams. the solution for it as it stands right now would be to move the environment changing as close to playback start (or stream creation) as possible as to reduce the risk of running in this override issue. a complete solution would require actually getting vlc/gst to set the custom property.

related IRC discussion:
<apachelogger> qputenv("PULSE_PROP_phonon.streamid", streamUuid.toLatin1());
<apachelogger> coling: is there a more sensible way to tell PA about our streamid?
<coling> apachelogger, not unless an API is made that bubbles down to the backends.
<apachelogger> right now we cannot have 2 working AOs :(
<apachelogger> so I guess we need the api
<coling> apachelogger, the reason it's injected like that is because the backends are abstracted and I have no way to tell VLC/GST etc. to inject a property nicely.
<apachelogger> j-b: fix your software plz
<apachelogger> coling: I figured that's the problem
<coling> apachelogger, why can we not have two?
<apachelogger> WELL
<coling> It should only be read when initialising the AO on the backend side IIRC.
<apachelogger> yes and there is the problem
<coling> Or perhaps it doesn't handle nicely streams change but we keep the same AO...
<apachelogger> what happens is that we set the env *when creating the frontend AO*, wheras it is forwarded to PA *when the gst/vlc create their stream*
<coling> So you create two AOs, and then they are initialised and both get the same stream UUID.
<coling> Hmmm.
<coling> Bummer.
<apachelogger> so we have AO1::AO1() -> uuid foo; AO2::AO2() -> uuid bar;
<apachelogger> vlc::createAO1(); vlc::createAO2();
<apachelogger> in both cases the envrionment says uuid bar
<coling> I'm not sure it's really the "fault" of the backends tho'. It's kinda expected that their implementation are hidden.
<apachelogger> what with AO2 now having overwritten AO1
-*- coling wonders what other cleverness can be done....
<apachelogger> coling: why do we need internal UUIDs anyway?
<apachelogger> I reckon if we did away with that and simply used PA's indexes we'd be fine
<coling> apachelogger, we basically need to get the PA stream id, such that we can control the stream properly. i.e. adjust volume etc. and do some other stuff (like force a device when testing).
<coling> apachelogger, I described it a bit here a while back: http://colin.guthr.ie/2009/10/so-how-does-the-kde-pulseaudio-support-work-anyway/
<coling> apachelogger, ignore that.
<coling> apachelogger, I didn't right about it in *that* much depth it seems.
--> DarthCodus_ (~anm@unaffiliated/darthcodus) hat #kde-multimedia betreten
<-- DarthCodus (~anm@unaffiliated/darthcodus) hat das Netzwerk verlassen (Ping timeout: 240 seconds)
<coling> apachelogger, but basically if the backends could supply us with a PA stream index number that would be lovely, but as the backend's audio outputs are also likely abstract, it's not something that I expect to bubble up through two layers of abstraction too easily.
<coling> It may be possible in gst via some kind of property on the sink object... not sure if that's exposed tho'.
<coling> apachelogger, perhaps a slightly better hack would be moving the qputenv() call around a bit?
<coling> e.g. to move it out of the constructor and into any methods that might create the stream... probably still doesn't work 100% of the time... (e.g. if the backend needs to tear down and recreate the stream without us knowing).
<coling> Not sure I have any brighter ideas.
<apachelogger> moving the qputenv would improve it, not a perfect solution though
<apachelogger> i.e. the backend should call it before it creates the thing that creates the stream
<apachelogger> BUT
<apachelogger> I am not sure that will work with VLC
<apachelogger> well
<apachelogger> technically it should
<apachelogger> *thinking*
<coling> There will almost definitely be corner cases where it will still break, but I guess breaking less often is still favourable.
<apachelogger> yeah should work since we need different mediaobjects and vlcplayers anyway, so there one would set the env before playing for example
<apachelogger> it's still time and syncness dependent though
<apachelogger> so it just reduces the scope of the bug, it doesn't really fix it
-*- coling nods
Comment 6 Harald Sitter 2013-06-17 23:09:36 UTC
Ah just to make it clear. A api user work around for this is to get the graphs into playing|paused|buffering subsequently. i.e. MediaObject1/AudioOutput1 need to be in a playing sub-state before one starts constructing MediaObject2/AudioOutput2.

so as an abstract example:
void setup()
{
VideoPlayer *vp = createVideoPlayer();
connect(vp, SIGNAL(statechanged), this, SLOT(statehandle));
vp->setCurrentSource("file:///foo.mp4");
vp->pause();
}

void setupTwo()
{
static bool init = false;
if (!init) {
  setup(); // create second videoplayer instance
  init = true;
}
}

void statehandle(old, new)
{
switch(new) {
case Playing: case Paused: case Buffering:
  setupTwo();
  break;
}
}
Comment 7 Harald Sitter 2013-07-01 12:14:21 UTC
Workaround as per discussion requires the following changes:

Phonon:
[API+] AOIf::StreamUuid (backed by AOP::streamUuid; not reflected in frontend AO)

Phonon VLC:
+ MO::setupMedia(...)
   traverse connected sinknodes, get all AOs
   if more than one AO (excluding ADO etc.), print error as this is not supported
   qputenv("PULSE_PROP_phonon.streamid", ....)
?  discuss the possibility of setting pulse streams at runtime as with gstreamer

Phonon GStraemer:
+ DM::createAudioSink(...)
   when creating a pulsessink set stream-properties [1]

[1] http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-good-plugins/html/gst-plugins-good-plugins-pulsesink.html#GstPulseSink--stream-properties
Comment 8 Harald Sitter 2013-07-24 13:44:48 UTC
Git commit 8b98d4e92c747b44eae0280fc9c271476775a068 by Harald Sitter.
Committed on 24/07/2013 at 13:39.
Pushed by sitter into branch 'master'.

support backend-driven pulseaudio property setting

- pulstream: new property role (== PA's media.role)
- pulsesupport: new func streamproperties
  returns a hash of per-stream properties a backend can set
- pulsesupport: new func setupstreamenvironment
  can be used by a backend not supporting per-stream properties to set
  the properties to the right value as close to stream creation as possible
  to reduce the scope of 'reentrancy' issues, howeve if two MO's are
  started right after one another the second AO's envrionment will still
  shadow the first's

app_name/version/icon_name are retained as always-on override properties
as we consider them static and therefore backend driven setting isn't
really necessary (also not possible with VLC<2.1)

when using pvlc (which uses the setup function due to lack of property
support through libvlc) a warning will be displayed in debug mode to
make clear what could happen.
FIXED-IN: 4.7.0

M  +7    -1    phonon/pulsestream.cpp
M  +4    -1    phonon/pulsestream_p.h
M  +67   -10   phonon/pulsesupport.cpp
M  +3    -1    phonon/pulsesupport.h

http://commits.kde.org/phonon/8b98d4e92c747b44eae0280fc9c271476775a068
Comment 9 Harald Sitter 2013-07-24 13:44:49 UTC
Git commit aa01e7e4b5fea95e0b2858337ceedddad18bc5de by Harald Sitter.
Committed on 24/07/2013 at 13:44.
Pushed by sitter into branch 'master'.

export AO's PA stream UUID to backend

- new interface class (go virtuals!) AudioOutputInterface47
  to use it the backend needs to define PHONON_BACKEND_VERSION_4_7
- new pure virtual in 47 ::setStreamUuid(QString) called right after
  backend creation to pass it the stream UUID used to map a PA stream
  to this AO; can be used to do property handling with PulseSupport

M  +4    -0    phonon/audiooutput.cpp
M  +34   -1    phonon/audiooutputinterface.h
M  +5    -1    phonon/phonondefs.h

http://commits.kde.org/phonon/aa01e7e4b5fea95e0b2858337ceedddad18bc5de
Comment 10 Harald Sitter 2013-07-24 14:07:19 UTC
Git commit 9ae98ed15ede1d829fbacf84078baf5733e3c4a4 by Harald Sitter.
Committed on 24/07/2013 at 14:00.
Pushed by sitter into branch 'master'.

implement PHONON_BACKEND_VERSION_4_7's setStreamUuid

due to present limitations in libvlc we cannot actually set the stream
properties interactively. instead setupStreamEnvironment is used to
set the envrionment as close as possible to creation.

also add fallback backend version checks, not sure we actually build with
< 4.6.60 though

M  +9    -1    src/CMakeLists.txt
M  +15   -0    src/audio/audiooutput.cpp
M  +4    -0    src/audio/audiooutput.h

http://commits.kde.org/phonon-vlc/9ae98ed15ede1d829fbacf84078baf5733e3c4a4
Comment 11 Harald Sitter 2013-07-24 14:07:24 UTC
Git commit 17928098f2a71623e1b1de61c06d258e0c8d81e6 by Harald Sitter.
Committed on 24/07/2013 at 13:55.
Pushed by sitter into branch 'master'.

implement PHONON_BACKEND_VERSION_4_7's setStreamUuid

we now interactively set PA per-stream properties through gstreamer
FIXED-IN: 4.7.0

M  +1    -1    gstreamer/CMakeLists.txt
M  +25   -0    gstreamer/audiooutput.cpp
M  +7    -1    gstreamer/audiooutput.h

http://commits.kde.org/phonon-gstreamer/17928098f2a71623e1b1de61c06d258e0c8d81e6
Comment 12 Harald Sitter 2013-07-24 21:03:14 UTC
Considered fixed for libphonon and phonon-gstreamer. Blocked in phonon vlc on support in libvlc.

Ultimately it was decided that PulseSupport should probably go away in Phonon5 as vlc/gstreamer should now be able to handle everything correctly.