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

List:       kde-devel
Subject:    Re: libphonon and kdebase4-runtime: Code advice/review
From:       Michael Pyne <mpyne () kde ! org>
Date:       2009-09-27 19:09:05
Message-ID: 200909271509.12428.mpyne () kde ! org
[Download RAW message or body]

[Attachment #2 (multipart/signed)]


On Sunday 27 September 2009 07:56:31 Colin Guthrie wrote:
> 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'm sorry we weren't able to help you more. A couple of things: During 
Matthias's studies Martin Sandsmark will be handling maintainer duties (see 
http://martinsandsmark.wordpress.com/2009/09/07/multimedia-frameworks-in-qt/ ) 
so he may be able to help out more with what I'm going to say.

Also, this list is for supporting KDE application developers. You're referring 
more to actually changing KDE libraries so the list you'd want would be more 
along the lines of kde-core-devel (or kde-multimedia or phonon-backends) You 
probably wanted kde-core-devel but I'll avoid cross-posting until we figure 
out what you need changed in KDE. :)

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

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

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

Sure (but do keep in mind that the application is not supposed to need to know 
or care about the ordering of devices at all. KDE is very much a "central 
planning" workspace. ;)

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

Probably a disagreement in ideals, the actual way our KCM module currently 
interfaces with Phonon configuration sounds like a hack but we do generally 
allow for KCM modules to be permitted a wider access to an underlying 
technology for configuration purposes (i.e. we don't necessarily limit KCM 
modules to the same interface we'd expect 3rd party apps to use)

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

Sounds fair. My question (I've read through your whole email so I apologize if 
it sounds like I'm not understanding something) is this: Do you intend for 
your changes to land upstream (here in KDE) or is this help for making changes 
for your distro work?

If you just need this to be available in your distribution it may be better to 
simply change the platform plugin itself, no? i.e. right now our platform 
plugin reads the list of devices, etc. picks one and tells Phonon to go forth 
and do great things. You could instead have the platform plugin either a) pick 
the appropriate PA device or b) do nothing and force the Phonon backend to 
pick a device. It seems to me this could be done without breaking ABI. Of 
course I think it would be hard to sell altering the KDE platform plugin to 
special-case PA upstream.

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

I'm sorry, but I didn't see exactly where the ABI broke.

For instance I saw a ABI mention in this commit to kdesupport: 
http://colin.guthr.ie/git/phonon/commit/?h=pulse&id=93e9fbac53fa39dd39f944c88e48e7078bab98c9
but you don't actually break ABI for C++ merely by adding a virtual method to 
a class that already has virtual methods. (On the other hand I'd recommend not 
inlining the method definition in the .h just to be safe).

C++ is annoying to deal with regarding ABI issues. You can see our KDE 
guidelines for "Is this BC?" here: 
http://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++

> 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/backendcapabiliti
> es.h?h=pulse (all methods except the first one are mine).

... yeah, you're probably right it's not the best place to put them. :)

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

Yeah, and now because we do it wrong by copying it makes it that much more of 
a problem trying to change Phonon's private global settings later.

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

Exported, yes. But I would only expect it to be used by the Phonon KCM 
(although if we export it we need to be ready for any Phonon-using app to get 
a hold of it ;)

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

It sounds acceptable to me. I don't have any background info on why it's not 
already exported, but the code looks like it's simply trying to defer to the 
backend and platform plugin.

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

Your phonon-gst fixes look intriguing. :)

Regards,
 - Michael Pyne

["signature.asc" (application/pgp-signature)]

>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<


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