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

[Dafydd Harries]

Ar 14/10/2008 am 16:25, ysgrifennodd Olivier Crête:
> First, thanks for the review. I fixed most of the problems that you
> found.
> 
> Extra questions/comments follow:
> 
> 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.

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.

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

> > +tp_stream_engine_audio_stream_make_src_bin (TpStreamEngineAudioStream *self);
> > 
> > I think it's better style if static functions don't have a prefix like this.
> > It means that you can see from a function name whether it's static or not
> > whithout having to look up the definition. Also, you use this style yourself with
> > functions like free_sinkbin(), so removing the prefix would also be more
> > consistent.
> 
> The general idea is that functions which are "methods" on the object
> have the prefix and those that are not don't.

Ok.

> > +  gst_object_unref (sink_pad);
> > +
> > +  gst_element_set_state (self->priv->srcbin, GST_STATE_PLAYING);
> > +
> > +  self->priv->src_pad_added_handler_id = g_signal_connect (self->priv->stream,
> > +      "src-pad-added", G_CALLBACK (src_pad_added_cb), self);
> > 
> > The blank lines in between these statements is wasted space. Remove them.
> 
> They're unrelated (and hence should be separated).

I can sort of see your logic, but I respectfully disagree. They are clearly
related to a degree given that they are adjacent. I think it's only worth
inserting extra blank lines in a long block of code, much as one only groups
sentences into paragraphs when there are lots of them.

Anyway, I don't care enough to make you change it.

> > +  binsink = gst_element_get_static_pad (bin, "sink");
> >  
> > +  sinkpeer = gst_pad_get_peer (binsink);
> > +  if (sinkpeer)
> > 
> > The blank line here should be before the if statement.
> 
> in this case, they're related and hence are not separated ;)

Whatevs.

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

> > The key file loading/error reporting code is repeated in this function. Factor
> > it out.
> 
> The code is slightly different enough to make that really painful :-(

Ah well. I guess this is C being sucky.

> > +  self->priv->audio_source_use_count--;
> > 
> > Does this not need to be atomic?
> 
> Its always called from the main thread

Ok.

> > +  if (self->priv->audio_sink_use_count <= 0)
> > 
> > Hmm. How might this be negative?
> 
> It shouldn't.. defensive programming?..

Ok.

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

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

> > +  if (g_object_has_property ((GObject *) element, "sync"))
> > +    g_object_set ((GObject *) element, "sync", FALSE, NULL);
> > +  if (g_object_has_property ((GObject *) element, "async"))
> > +    g_object_set ((GObject *) element, "async", FALSE, NULL);
> > 
> > This seems self-contradictory. Surely one of those FALSEs should be a TRUE.
> 
> No, no, they both are false, we can not sync on the send side of the
> pipeline because current Gst pipelines can only have one latency, and it
> would force it to have the same latency as the receive side (and would
> therefore suck). Having a MultiBin is on my todo list..

Ok.

> > Why aren't errors from the video preview's make_sink() propagated?
> 
> There is no point in using a GError for a plugin installation error (its
> not a user recoverable error).

Ok.

> > You could use libnice style, btw. (Each parameter is indented to four spaces.)
> 
> I couldn't agree more... the telepathy style is fail

:)

> > +  if (!self->priv->fs_conference)
> > +    {
> > +      g_error ("Invalid session");
> > +      return obj;
> > +    }
> > 
> > This code makes no sense. g_error always aborts the process, so the return
> > never does anything. Perhaps you want to propagate an error?
> 
> I'm not sure how to propagate it properly in a subclassing friendly way.
> But g_error is definitely wrong

Hmm. Can we call up to a superclass error function or something?

> > +  stream->priv->fs_stream = fs_session_new_stream (stream->priv->fs_session,
> > +      stream->priv->fs_participant,
> > +      FS_DIRECTION_NONE,
> > +      transmitter,
> > +      n_args,
> > +      params,
> > +      &stream->priv->construction_error);
> > +
> > +  if (!stream->priv->fs_stream)
> > +    return obj;
> > 
> > Shouldn't you propagate any error returned here?
> 
> Its already propagated, the fs_session_new_stream() function will write
> it into our construction_error field.

Sorry, I thought I'd deleted this part of the review.

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

-- 
Dafydd