KDE/kdelibs/phonon

[Matthias Kretz]

On Friday 14 March 2008, Thierry Bastian wrote:
> Hello Ian,
>
> I just wonder what review board you're talking about...

He means the two mails that he sent to phonon-backends linking to the review 
board website where you can see and comment on the patch (after registering). 
It was a bit of an experiment to use the website for this.

> Anyway, here are the points I found about this patch.
>
> Good ones:
> - it removes a load of commented code in the most used classes (I hate
> commented code)
> - it adds those functionality as optional ones thanks to MediaController so
> that it doesn't break the whole thing.
>
> Bad points:
> - the functions you uncommented are old and reference videoPath and
> audioPath which don't even exist any more.

You're right, of course. I didn't take enough time to think about it and only 
remembered my old thoughts (pre-graph-discussions) on this. Ian only added 
the path arguments because I told him to keep them. My fault.

> - I think submitting something without having the implementation behind
> (even a proof-of-concept) is rarely a good idea.

Ian has an implementation for phonon-xine + API use in dragonplayer. Except 
for the path arguments...

> For the videoPath/audioPath, I suggest the following change we use the
> Phonon::Path class instead. I'll submit that patch to svn ASAP.

On Friday 14 March 2008, Thierry Bastian wrote:
> It goes against what we already agreed on: it is not on the path that we
> set which audio/video stream should be used. We do that on the outputs of
> the mediaobject (the ports). By default , the MediaObject has one output
> for each type of media (Audio and Video for now, subtitle in the future).

Wow. Seems my brain is a black hole. I'm glad I still have 
phonon/DESIGN/Phonon-Graph.cpp to get those thoughts back. OK, so we want to 
have the following:

MediaObject *media = new MediaObject;
media->setCurrentSource(Phonon::Dvd);
VideoWidget *vwidget = new VideoWidget(parent);
AudioOutput *aoutput = new AudioOutput;
Path path = Phonon::createPath(media, vwidget);
// path.mediaStreamTypes() == Phonon::Video | Phonon::Subtitles
Phonon::createPath(media, aoutput);
QList<SubtitleStreamDescription> l = media->availableSubtitleStreams();
// either:
media->setCurrentSubtitleStream(l.first());
// or:
Port port1 = media->openPort(Phonon::Subtitles);
media->setCurrentSubtitleStream(l.at(0), port1);
Port port2 = media->openPort(Phonon::Subtitles);
media->setCurrentSubtitleStream(l.at(1), port2);

// in the latter case you probably want an explicit path for subtitles:
path.reconnect(media, Port(Phonon::Video), vwidget);
Phonon::createPath(media, port1, vwidget);
VideoWidget *vwidget2 = new VideoWidget(parent);
Phonon::createPath(media, Port(Phonon::Video), vwidget2);
Phonon::createPath(media, port2, vwidget2);

Is that what we wanted?

Then some of the work Ian did needs to move back from MediaController to 
MediaObject and all the path references in those functions need to be 
removed.

What about the video angle functions in MediaController? Should we deprecate 
them and add them as *VideoStream* functions in MediaObject?

-- 
________________________________________________________
Matthias Kretz (Germany)                            <><
http://Vir.homelinux.org/
MatthiasKretz at gmx.net, kretz at kde.org,
Matthias.Kretz at urz.uni-heidelberg.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/phonon-backends/attachments/20080314/7a7342d8/attachment.pgp