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

[Dafydd Harries]

Ar 17/10/2008 am 14:58, ysgrifennodd 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:

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

I think the correct thing to do is to use sysconfdir from autoconf.

> > > > +#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 $ 

Sorry, I mean g_build_filename(). Not too important anyway.

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

Cool. :)

If the config file thing is changed, I'm happy with the branch. I've created
a telepathy-farsight repository for you to merge to.

-- 
Dafydd