Review Request: Refactor libtelepathy-kde-call, the audio/video streaming library of the call ui.

Wed Mar 30 17:51:24 CEST 2011

> On March 30, 2011, 2:52 p.m., Dario Freddi wrote:
> > CMakeLists.txt, line 13
> > <http://git.reviewboard.kde.org/r/100687/diff/1/?file=9527#file9527line13>
> >
> >     Was about to yell "PKGCONFIG!!" when I saw your comment, so ok, if you really mean it :) otherwise, try and draft a quick cmake find.

I'd prefer if upstream (you in this case?) added a TelepathyQt4*Config.cmake file :)

> On March 30, 2011, 2:52 p.m., Dario Freddi wrote:
> > libqtf/CMakeLists.txt, lines 27-28
> > <http://git.reviewboard.kde.org/r/100687/diff/1/?file=9528#file9528line27>
> >
> >     LDFLAGS->LIBRARIES: typo?

It's not a typo, pkg_check_modules creates those variables.

> On March 30, 2011, 2:52 p.m., Dario Freddi wrote:
> > libqtf/qtf.cpp, lines 37-42
> > <http://git.reviewboard.kde.org/r/100687/diff/1/?file=9530#file9530line37>
> >
> >     Watch out for correct indentation

Right, this was copy-paste from qt-gstreamer's codegen output, which uses 2-space indentation. I'll adjust it, although it will probably not stay for long (I will make codegen public soon, so that other projects can use it at build time to generate this code).

> On March 30, 2011, 2:52 p.m., Dario Freddi wrote:
> > libtelepathy-kde-call/callchannelhandler.h, line 2
> > <http://git.reviewboard.kde.org/r/100687/diff/1/?file=9532#file9532line2>
> >
> >     Nitpicking: you should change to 2010-2011 instead of changing the year completely when talking about copyrights

Well, this class was actually rewritten from scratch (I threw out the old files and started from empty ones), but since it shares the same name with an older class, it appears in the diff like this.

> On March 30, 2011, 2:52 p.m., Dario Freddi wrote:
> > libtelepathy-kde-call/callchannelhandler.h, line 51
> > <http://git.reviewboard.kde.org/r/100687/diff/1/?file=9532#file9532line51>
> >
> >     code style: const * d;
> >     Also, you'd be better off calling this class Private, given that it is declared inside the scope of its parent class. If you want to preserve the naming, you could move it outside.

You sure about T const * d? Isn't it  T * const d; ? At least that's what it says here: http://techbase.kde.org/Policies/Library_Code_Policy#D-Pointers

Or maybe you are referring to the missing space? In this case, I'll fix it.

As for the name, this class is not declared inside the scope of this class, see the forward declaration above. I'd prefer to keep it that way, since it's a standalone QObject.

> On March 30, 2011, 2:52 p.m., Dario Freddi wrote:
> > libtelepathy-kde-call/callcontenthandler.h, line 29
> > <http://git.reviewboard.kde.org/r/100687/diff/1/?file=9535#file9535line29>
> >
> >     Try and use /** instead of /*! when writing docs

ok...

> On March 30, 2011, 2:52 p.m., Dario Freddi wrote:
> > libtelepathy-kde-call/callcontenthandler.cpp, line 45
> > <http://git.reviewboard.kde.org/r/100687/diff/1/?file=9536#file9536line45>
> >
> >     code style: Q_FOREACH (

ok

> On March 30, 2011, 2:52 p.m., Dario Freddi wrote:
> > libtelepathy-kde-call/callcontenthandler.cpp, line 49
> > <http://git.reviewboard.kde.org/r/100687/diff/1/?file=9536#file9536line49>
> >
> >     Danger ahead: be sure to notice that sender() might be invalid when connecting a slot to this signal.

You mean because I call deleteLater() afterwards? Yes, sender might be invalid if the slot is connected with Qt::QueuedConnection or from another thread, but in this case, this object is private and it is only used with Qt::DirectConenction here, so it's ok. I am aware of it.

> On March 30, 2011, 2:52 p.m., Dario Freddi wrote:
> > libtelepathy-kde-call/callcontenthandler.cpp, line 96
> > <http://git.reviewboard.kde.org/r/100687/diff/1/?file=9536#file9536line96>
> >
> >     Code style: avoid using brackets in case:

Right. These are remainders from an older version where I had to declare variables inside the case. I'll remove them.

> On March 30, 2011, 2:52 p.m., Dario Freddi wrote:
> > libtelepathy-kde-call/sinkcontrollers.h, lines 31-32
> > <http://git.reviewboard.kde.org/r/100687/diff/1/?file=9540#file9540line31>
> >
> >     Try and file an upstream bug, it definitely might have its place up there.

Yep.

> On March 30, 2011, 2:52 p.m., Dario Freddi wrote:
> > libtelepathy-kde-call/sinkcontrollers.h, line 46
> > <http://git.reviewboard.kde.org/r/100687/diff/1/?file=9540#file9540line46>
> >
> >     Why is the destructor protected?

Because I only want BaseSinkManager to be able to create and destroy these objects. User code must not be able to do that.

> On March 30, 2011, 2:52 p.m., Dario Freddi wrote:
> > libtelepathy-kde-call/sinkcontrollers.h, line 52
> > <http://git.reviewboard.kde.org/r/100687/diff/1/?file=9540#file9540line52>
> >
> >     Missing destructor

Not required, but if this becomes a shared lib it should be added for keeping BC, so ok...

> On March 30, 2011, 2:52 p.m., Dario Freddi wrote:
> > libtelepathy-kde-call/sinkcontrollers.cpp, line 29
> > <http://git.reviewboard.kde.org/r/100687/diff/1/?file=9541#file9541line29>
> >
> >     You'd be better off in using Q_DECLARE_PUBLIC, which gives inheritance goodness to the q pointer as well.

Err, right. This is why I named the q pointer q_ptr, but probably I forgot to add Q_DECLARE_PUBLIC because I didn't need to use Q_Q anywhere.

> On March 30, 2011, 2:52 p.m., Dario Freddi wrote:
> > libtelepathy-kde-call/sinkcontrollers.cpp, lines 143-146
> > <http://git.reviewboard.kde.org/r/100687/diff/1/?file=9541#file9541line143>
> >
> >     You could think about switching to QReadWriteLock instead of QMutex here

Nah, this will make the code even more complicated. This code is not performance-critical anyway, it will be called only a few times (1-2 times for simple 1:1 calls) for the whole duration of the call and in most cases the mutex is not even needed, in the sense that the whole operation will be finished when this method is called again, however I have to have the mutex there for safety.

> On March 30, 2011, 2:52 p.m., Dario Freddi wrote:
> > libtelepathy-kde-call/tests/CMakeLists.txt, lines 1-2
> > <http://git.reviewboard.kde.org/r/100687/diff/1/?file=9546#file9546line1>
> >
> >     That's probably the only real issue I see here: this class should be moved indeed to be an autotest.

The problem here is that there is nothing to check programatically in this test. The check is done by listening if audio is stopped and resumed correctly, so this test needs to run manually. I added it basically for experimenting with gstreamer, because I wasn't sure if the trick I did would work as expected.

> On March 30, 2011, 2:52 p.m., Dario Freddi wrote:
> > libqtf/CMakeLists.txt, lines 4-5
> > <http://git.reviewboard.kde.org/r/100687/diff/1/?file=9528#file9528line4>
> >
> >     If those are meant to stay, you need a cmake module for that (I know, I feel your pain :( )

Yeah, those are to stay. It's quite a PITA to do this in cmake because the dependencies actually are:

* telepathy-farstream
* farstream
* gstreamer
* telepathy-glib
* dbus-glib
* libxml2
* gio
* gobject
* glib
* telepathy-qt4-yell-farstream

I don't think pkgconfig is bad in this case, because it really solves a problem here and since this code will not work on any other platform than unix/x11 at the moment (requires Qt glib event loop integration), there is no point in arguing that there will be build problems on windows/mac (which is the main reason for not using pkgconfig, right?)

- George

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100687/#review2257
-----------------------------------------------------------

On Feb. 19, 2011, 10:20 p.m., George Kiagiadakis wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100687/
> -----------------------------------------------------------
> 
> (Updated Feb. 19, 2011, 10:20 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Summary
> -------
> 
> This is a complete refactoring of libtelepathy-kde-call, the library that handles audio/video streaming using telepathy-farstream.
> 
> The new design is based on the new Call.DRAFT spec and telepathy-farstream, which are way cleaner than their predecessors, StreamedMediaChannel and telepathy-farsight. This inherently makes the design of this library cleaner too. In addition to this, the GStreamer sources and sinks have also been redesigned to handle correctly possible race conditions (i.e. do correct synchronization between the main thread and the gstreamer streaming threads and also between telepathy-qt4 and telepathy-glib) and also allow dynamic switching of source devices.
> 
> http://quickgit.kde.org/?p=clones/telepathy-call-ui/gkiagia/telepathy-call-ui.git&a=shortlog&h=refs/heads/call-2
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 59582bfa98fd738e50a5efbb39d03802e9f0e25c 
>   libqtf/CMakeLists.txt PRE-CREATION 
>   libqtf/qtf.h PRE-CREATION 
>   libqtf/qtf.cpp PRE-CREATION 
>   libtelepathy-kde-call/CMakeLists.txt d6a39b8d182f457909a78bf9a02a0d0efbf1aceb 
>   libtelepathy-kde-call/callchannelhandler.h 94c27c18a1c37f590232acd9c9a77a969f85bedd 
>   libtelepathy-kde-call/callchannelhandler.cpp 05aeff6040bb741a61b08ff1520e002487903406 
>   libtelepathy-kde-call/callchannelhandler_p.h 5a1ce85bee81ef0e3c088e22845537e9c60c62f0 
>   libtelepathy-kde-call/callcontenthandler.h PRE-CREATION 
>   libtelepathy-kde-call/callcontenthandler.cpp PRE-CREATION 
>   libtelepathy-kde-call/callcontenthandler_p.h PRE-CREATION 
>   libtelepathy-kde-call/callparticipant.h 5ac2ad7d69e5465f18f25de0c86b8333ee632af3 
>   libtelepathy-kde-call/callparticipant.cpp 9a683935d1212262671b6bab071b179b70dacb5b 
>   libtelepathy-kde-call/sinkcontrollers.h PRE-CREATION 
>   libtelepathy-kde-call/sinkcontrollers.cpp PRE-CREATION 
>   libtelepathy-kde-call/sinkcontrollers_p.h PRE-CREATION 
>   libtelepathy-kde-call/sourcecontrollers.h PRE-CREATION 
>   libtelepathy-kde-call/sourcecontrollers.cpp PRE-CREATION 
>   libtelepathy-kde-call/sourcecontrollers_p.h PRE-CREATION 
>   libtelepathy-kde-call/tests/CMakeLists.txt PRE-CREATION 
>   libtelepathy-kde-call/tests/sourcetest.cpp PRE-CREATION 
>   libtelepathy-kde-call/volumecontroller.h PRE-CREATION 
>   libtelepathy-kde-call/volumecontroller.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/100687/diff
> 
> 
> Testing
> -------
> 
> I have successfully done audio calls with call-ui <-> empathy and call-ui <-> echo bot, using a modified version of the GUI that will be in another review request later, once it is completed. Video calls and some other features are not tested, as the GUI still needs some work to support them.
> 
> 
> Thanks,
> 
> George
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-telepathy/attachments/20110330/545bef0a/attachment-0001.htm 