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

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

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


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


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

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

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

> 
> 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 :-(


> +  self->priv->audio_source_use_count--;
> 
> Does this not need to be atomic?

Its always called from the main thread

> +  if (self->priv->audio_sink_use_count <= 0)
> 
> Hmm. How might this be negative?

It shouldn't.. defensive programming?..

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

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


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


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


> 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


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


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

-- 
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/20081014/9bcccad9/attachment.pgp