[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-devel
Subject:    libphonon and kdebase4-runtime: Code advice/review
From:       Colin Guthrie <gmane () colin ! guthr ! ie>
Date:       2009-09-27 11:56:31
Message-ID: h9njti$dhp$1 () ger ! gmane ! org
[Download RAW message or body]

Hi Everyone,

If you follow certain circles you'll know what I'm working on, but for 
background please see this thread:
http://mail.kde.org/pipermail/phonon-backends/2009-September/000304.html

Now sadly I didn't get the information I needed from the above post (the 
mailing list is not super active) and I fear that Matthias is the only 
person who could answer authoritatively and I'd imagine he'll be "nose 
to the grindstone" on various study related things so I'm asking here 
for some advice and assistance.


I've given a detailed background in the above linked mail so I wont do 
it again, I'll just summarise shortly here.

The problem is thus:

The current libphonon + KDE Platform Plugin interaction is very strange 
(read: bad design).

When presenting the GUI, the (kdebase4-runtime) code will read in the 
libphonon config file and interpret it's device priority list for each 
category. It will then ask the platform plugin for it's devices and then 
the backend for it's devices and combine the lists with the order from 
the config to create an ordered list of devices for display.


Now, when some audio is played, the backend code (libphonon) will read 
in the libphonon config file and interpret it's device priority list for 
each category. It will then ask the platform plugin for it's devices and 
then the backend for it's devices and combine the lists with the order 
from the config to create an ordered list of devices so it can pick the 
appropriate device for playing the current audio.


Now there is obviously a big parallel there, with frontend code reading 
backend config and doubling up on the logic for ordering the devices.

This is IMO bad design for two reasons:
  1. The frontend client code has to have knowledge of how libphonon 
stores it's configuration and basically tweaks it's config for it.

It should not do that. How libphonon does this should be a black box. If 
it wants to use a config file, great it can do but the frontend should 
not be aware of it, nor need to read or adjust it in order to get the 
information it needs.

  2. Frontend code has to be aware of the platform plugin to list it's 
devices in an appropriate order - if I write a generic phonon app that 
lists devices, I would want to say "hey libphonon gimme all your 
devices, in order, for this category please" and have libphonon say 
"sure, here you go". I don't want my application to be intimately away 
of the whole concept of platform plugins - this should be abstracted 
completely by libphonon. I don't think the config GUI in KDE should be 
any different.



Now the reason this causes me problems (I'd normally just look the other 
way!) is that I want to override the use of the platform plugin from 
libphonon. I want the phonon engine itself to say "actually, in this 
specific case, the platform plugin does not know better about audio 
devices than me, so I'm not going to ask it." I do not want to propigate 
through a flag from the backend right the way up through the API to all 
client applications so that the client applications can combine the two 
lists of devices appropriately. I just want the backend to be the only 
place used to dish out all devices and their priority in the first place 
and be able to do so in order on a per-category basis.


Now I've been doing a lot of hacking on this and have more or less 
achieved this design goal, but it comes at a price: ABI breakage. Now I 
need to know how best to fit this in and what bit I should tweak in my 
current patches.

My current patch changes only internal code, with the exception of a few 
additional exported calls in Phonon::Experimental::BackendCapabilities 
namespace. This is not really an appropriate namespace for the functions 
in question, but it was the easiest place to put them!
Here is the public API tweak as it stands:
http://colin.guthr.ie/git/phonon/tree/phonon/experimental/backendcapabilities.h?h=pulse
(all methods except the first one are mine).

There are lots of internal changes, especially to 
phonon/phonon/globalsettings.cpp This class is private, i.e. not 
exported to the clients. (currently, this class is also copied to the 
frontend code which is IMO, as I mentioned above, bad design - you 
shouldn't work around a non-exported class in a library by copying it - 
you need to address the design problems properly!).

So in my libphonon changes, I've made this class able to be queried in 
such a way that I can get useful listings for devices on a per-category 
basis from it. It is becomes the sole class at the backend responsible 
for interpreting/writing it's own config file and providing devices 
indexes in the appropriate order for a given category etc. I've added 
calls to tweak e.g. the hiding/showing of advanced devices and to write 
updated priority lists.

IMO, this class needs to be exported in some capacity (replacing my 
backendcapabilites API) to allow clients the ability to present GUIs 
appropriately, and allow the user to change the configuration, while 
remaining abstract as to how the actual storage should be.

With the current changes I've been able to modify the runtime components 
such that the copy of globalconfig.cpp is removed and only the backend 
is asked for device listings and all configuration changes are passed 
through these wrappers.


So, here is the question: Can we export the backend's globalconfig? If 
this is done, I wouldn't need my wrappers in the BackendCapabilities 
namespace. It doesn't change the API in any major way (e.g. it only adds 
to it - it does not change it) and would allow IMO a cleaner design. If 
this is possible I'd propose to clean up the API a bit, so we can be 
confident going forward that it will meet everyone's needs. IMO 
configuring libphonon's operation should be as much part of the API as 
just playing sound. That's not to say that globalconfig in it's current 
form is suitable for export but if at least the principle is sound, that 
would be good to know going forward.


Thanks for reading and I hope I've explained my needs accurately.


Here are my current code changes:
http://colin.guthr.ie/git/phonon/log/?h=pulse
http://colin.guthr.ie/git/runtime/log/?h=pulse

(PS ignore the first runtime commit - it's subsequently been superseded 
and is the main problem I was concerned about in point 1 above!)

These changes require an additional pulseaudio module here:
http://colin.guthr.ie/git/pulseaudio/log/?h=history


Cheers

Col

-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
   Tribalogic Limited [http://www.tribalogic.net/]
Open Source:
   Mandriva Linux Contributor [http://www.mandriva.com/]
   PulseAudio Hacker [http://www.pulseaudio.org/]
   Trac Hacker [http://trac.edgewall.org/]

 
>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<
[prev in list] [next in list] [prev in thread] [next in thread]