[Telepathy] review: stream engine 'fs2-lib' branch

[Olivier Crête]

On Fri, 2008-10-17 at 17:54 +0100, Dafydd Harries wrote:
> Ar 14/10/2008 am 16:25, ysgrifennodd Olivier Crête:
> > On Fri, 2008-10-10 at 16:06 +0100, Dafydd Harries wrote:
> > > -service_in_files = org.freedesktop.Telepathy.StreamEngine.service.in
> > > +service_in_files = org.maemo.Telepathy.StreamEngine.service.in
> > > 
> > > Hmm, this seems like a slightly odd choice of name.
> > 
> > Any other suggestions? The goal is to make it clear that the
> > out-of-process handling is really a maemo-specific thing.
> 
> While SE's past is exclusively in maemo land, and its future might well be
> exclusively there also, there are non-Maemo people using it too, however
> limited it is. The fact that your new code has #ifdef MAEMO_SUPPORT in it
> attests to this.

Hopefully, the ifdef MAEMO_SUPPORT support will be going away. The idea
is to scare potential users into using the library.

> But ok, I don't really care.
> 
> A more interesting question is where your branch should go. I think it might
> be best to copy the SE repository to a telepathy-farsight repository and merge
> it there.

Yes, I think we should split the telepathy-farsight library from the
stream-engine package.


> > > +confdir = $(sysconfdir)/xdg/stream-engine
> > > 
> > > I don't think gstcodecs.conf should be in /etc/xdg. Put it in /etc/telepathy/stream-engine.
> > > 
> > > Is there also a global config file for farsight2? If so, the SE one should
> > > inherit from it somehow, to avoid duplication.
> > 
> > No, Farsight 2 has no config file, the config is passed programatically.
> > 
> > I'm putting it into /etc/xdg because that's what
> > g_get_system_config_dirs() returns and that follows the 
> > "XDG Base Directory Specification"
> 
> I don't really understand why g_get_system_config_dirs() returns /etc/xdg. But
> it seems to me that the XDG base directory specification is relevant only for
> reference by other XDG specifications.
> 
> The FHS says that config files go in /etc.

I know.... I'm a bit lost in the whole XDG thing too


> > > +#include <telepathy-farsight/channel.h>
> > > +#include <telepathy-farsight/stream.h>
> > > 
> > > I think these includes should use "" instead of <>, since the headers should
> > > be read from the build tree, not from the system.
> > 
> > I changed that, but we have to not forget to revert it when we split the
> > library from maemo-s-e.
> 
> Wouldn't we get compilation errors if we forgot?

I guess

> > > It looks like there's a lot of similar path building code spread across
> > > various files. I think it would be good to factor it out.
> > 
> > What path building?
> 
> g_build_path() calls.

Where ?
tester at TesterTop3 ~/collabora/telepathy-stream-engine $ grep -nrI g_build_path  *
tester at TesterTop3 ~/collabora/telepathy-stream-engine $ 

> > > +        gst_element_set_state (engine->priv->pipeline, GST_STATE_NULL);
> > > +        gst_element_set_state (engine->priv->videosrc, GST_STATE_NULL);
> > > +        gst_element_set_state (engine->priv->pipeline, GST_STATE_PLAYING);
> > > 
> > > Interesting. How does this work? Doesn't it run the risk of an infinite {
> > > error -> stop -> start } loop?
> > 
> > It does, but in that case, you're fully screwed anyway. Its useful for
> > when you have a recoverable error.. I have no idea how to differentiate
> > fatal and non-fatal errors.
> 
> Ah, I see. Doesn't GStreamer separate errors into critical/non-critical ones?

Well, they are errors that are critical to gstreamer, but may possibly
be recovered from by restarting the pipeline. Stuff like the video
driver being crappy, crashing.. Closing and re-opening the device often
helps.


> > > You sometimes indent lables to 0, 1, or 2 spaces. Pick one.
> > 
> > I believe it should be at "-1" from the current code, I haven't found
> > any that wasn't consistent.
> > 
> > > Consider using the code in other Telepathy components for automatically
> > > generating signal marshaller list files.
> > 
> > I tried.. and failed..
> 
> What happened?

I obviously did something wrong, I copied the makefile stanza from
gabble and it just worked... 


-- 
Olivier Crête
olivier.crete at collabora.co.uk
Collabora Ltd
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
Url : http://lists.freedesktop.org/archives/telepathy/attachments/20081017/a10d7f9c/attachment.pgp