[Telepathy] review: stream engine 'fs2-lib' branch
Dafydd Harries
dafydd.harries at collabora.co.uk
Fri Oct 10 08:06:31 PDT 2008
+ <tp:docstring>
+»·······Called to tell Stream Engine to Attach itself to a channel.
+ </tp:docstring>
Remove this tab.
doc/Makefile \
+»······· doc/lib/Makefile \
+ telepathy-farsight/Makefile \
+»······· telepathy-farsight/telepathy-farsight.pc \
src/Makefile \
Remove these tabs.
-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.
+# This is a blank Makefile.am for using gtk-doc.
+# Copy this to your project's API docs directory and modify the variables to
+# suit your project. See the GTK+ Makefiles in gtk+/docs/reference for examples
+# of using the various options.
Remove this.
+ <releaseinfo>
+ The latest version of this documentation can be found on-line at
+ <ulink role="online-location" url="http://telepathy.freedesktop.org/">http://telepathy.freedesktop.org/</ulink>.
+ </releaseinfo>
This should point directly to http://telepathy.freedesktop.org/doc/<whatever>.
+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.
Why the PROP_0 enum members? If the first enum member needs to not be zero for
some reason, do:
enum {
PROP_STREAM = 1,
PROP_BIN,
...
You do this already for TfChannel; be consistent.
+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 audio stream constructor catches various errors (adding source to bin,
getting src/sink pads, linking src to bin, etc.) but doesn't seem to propagate
them in any way. Surely an error condition should be signalled. This is
presumably what the private construction_error member is for, but it doesn't
seem to be used at all.
+ gst_object_unref (src_pad);
+ gst_object_unref (sink_pad);
+
+
+
+ sink_pad = gst_element_get_static_pad (self->priv->srcbin, "sink");
Three blank lines? Remove them.
+ 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.
+ 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.
+ param_spec = g_param_spec_object ("stream",
+ "Tp StreamEngine Stream",
+ "The Telepathy Stream Engine Stream",
+ TF_TYPE_STREAM,
+ G_PARAM_CONSTRUCT_ONLY |
+ G_PARAM_READWRITE |
+ G_PARAM_STATIC_NICK |
+ G_PARAM_STATIC_BLURB);
+ g_object_class_install_property (object_class, PROP_STREAM, param_spec);
I feel that the temporary param_spec variable here isn't really benficial, but
don't care a great deal.
+ signals[REQUEST_PAD] = g_signal_new ("request-pad",
+ G_OBJECT_CLASS_TYPE (klass),
+ G_SIGNAL_RUN_LAST | G_SIGNAL_DETAILED,
+ 0,
+ request_pad_accumulator, NULL,
+ tp_stream_engine_marshal_OBJECT__VOID,
+ GST_TYPE_PAD, 0);
Indentation should be four spaces here, not 16.
+ g_debug ("set input volume");
I don't think this is useful. If it's useful for this property, surely there
should be a print for the other properties too.
+ param_spec = g_param_spec_object ("stream",
+ "Tp StreamEngine Stream",
+ "The Telepathy Stream Engine Stream",
+ TF_TYPE_STREAM,
+ G_PARAM_CONSTRUCT_ONLY |
+ G_PARAM_READWRITE |
+ G_PARAM_STATIC_NICK |
+ G_PARAM_STATIC_BLURB);
+ g_object_class_install_property (object_class, PROP_STREAM, param_spec);
I feel that the temporary param_spec variable here isn't really benficial, but
don't care a great deal.
+ if (!gst_element_add_pad (bin, gst_ghost_pad_new ("src", pad)))
Don't you leak the ghost pad here if adding it fails? The code in
src_pad_added_cb() seems to handle a similar case properly.
+ if (!gst_element_add_pad (bin, gst_ghost_pad_new ("sink", pad)))
Same here.
+ g_return_val_if_fail (TP_STREAM_ENGINE_IS_STREAM (stream) &&
Given that stream is a TfStream *, shouldn't this check be TF_IS_STREAM?
+ audioconvert = gst_element_factory_make ("audioconvert", NULL);
Indentation should be two, not four.
+#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.
+ g_debug ("Could not read element properties config at %s: %s",
filename,
+ error ? error->message : "");
s/""/"(no error message set)"/
The key file loading/error reporting code is repeated in this function. Factor
it out.
+ self->priv->audio_source_use_count--;
Does this not need to be atomic?
+ if (state_ret == GST_STATE_CHANGE_FAILURE)
+ g_error ("Error starting the video source");
g_error() will abort the process. Is this really suitable?
+ sestream = g_object_get_data (G_OBJECT (stream), "se-stream");
+ if (sestream)
+ {
You should just do an early return here, as you have a long if clause and no
else clause.
+ if (self->priv->audio_sink_use_count <= 0)
Hmm. How might this be negative?
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.
+close_one_stream (TfChannel *chan G_GNUC_UNUSED,
+ guint stream_id G_GNUC_UNUSED,
+ TfStream *stream,
+ gpointer user_data)
Indent fail.
+ tf_stream_error (stream, TP_MEDIA_STREAM_ERROR_UNKNOWN,
+ message);
This smells a bit funny. Either there should be a better way to close a
stream, or this function should be called error_one_stream().
+ 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?
- g_object_set (videosrc, "is-live", TRUE, NULL);
if (videosrc == NULL)
g_error ("failed to create any video source");
+ g_object_set (videosrc, "is-live", TRUE, NULL);
Haha.
- GError e = { TP_ERRORS, TP_ERROR_NOT_AVAILABLE,
- "Window ID has already been removed" };
+ GError *error = g_error_new (TP_ERRORS, TP_ERROR_NOT_AVAILABLE,
+ "Could not get a video source");
+ g_debug ("%s", error->message);
- dbus_g_method_return_error (context, &e);
+ dbus_g_method_return_error (context, error);
+ g_error_free (error);
There doesn't seem to be any reason to not stack allocate this.
tp_stream_engine_mute_output (StreamEngineSvcStreamEngine *iface,
- const gchar *channel_path,
- guint stream_id,
- gboolean mute_state,
- DBusGMethodInvocation *context)
+ const gchar *channel_path,
+ guint stream_id,
+ gboolean mute_state,
+ DBusGMethodInvocation *context)
Eh?
--- /dev/null
+++ b/src/videopreview.c
@@ -0,0 +1,376 @@
+/*
+ * stream.c - Source for TpStreamEngineVideoStream
The filenames and description don't match.
+ 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.
The casts in the g_object_set() calls are unnecessary because it takes a
gpointer as the first argument. Probably g_object_has_property() should do
this too if it doesn't already.
Why aren't errors from the video preview's make_sink() propagated?
+ self->priv->construction_error = g_error_new (TP_ERRORS, TP_ERROR_PERMISSION_DENIED,
+ "Could set sink to playing");
ITYM "Couldn't".
+ /* hack to leave an xvimage free for the bigger output.
+ * Most machines only have one xvport, so this helps in the majority of
+ * cases. More intelligent widgets
+ * */
This comment seems incomplete.
+tp_stream_engine_video_stream_finalize (GObject *object)
This function should upcall to the parent class.
+#define CHANNEL_PRIVATE(o) ((o)->priv)
I don't see the use of macros like this if you have ->priv.
+static void
+tf_channel_get_property (GObject *object,
+ guint property_id,
+ GValue *value,
+ GParamSpec *pspec)
+{
Indentation fail.
+static GObject *
+tf_channel_constructor (GType type,
+ guint n_props,
+ GObjectConstructParam *props)
Again.
You could use libnice style, btw. (Each parameter is indented to four spaces.)
+TfChannel *
+tf_channel_new (TpDBusDaemon *dbus_daemon,
+ const gchar *bus_name,
+ const gchar *connection_path,
+ const gchar *channel_path,
+ guint handle_type,
+ guint handle,
+ GError **error)
More.
+ 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?
+ param_spec = g_param_spec_object ("proxy", "TpMediaSessionHandler proxy",
+ "The session handler proxy which this session interacts with.",
+ TP_TYPE_MEDIA_SESSION_HANDLER,
+ G_PARAM_CONSTRUCT_ONLY | G_PARAM_READWRITE |
+ G_PARAM_STATIC_NICK | G_PARAM_STATIC_BLURB);
+ g_object_class_install_property (object_class, PROP_PROXY, param_spec);
This property uses a totally different style to the other properties in this
function. I actually prefer this style, but either is fine as long as you're
consistent.
+/**
+ * _tf_session_bus_message:
+ * @session: A #TfSession
This docstring doesn't match the name of the function it documents.
+ 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?
+ param_spec = g_param_spec_object ("proxy", "TpMediaStreamHandler proxy",
+ "The stream handler proxy which this stream interacts with.",
+ TP_TYPE_MEDIA_STREAM_HANDLER,
+ G_PARAM_CONSTRUCT_ONLY | G_PARAM_READWRITE |
+ G_PARAM_STATIC_NICK | G_PARAM_STATIC_BLURB);
+ g_object_class_install_property (object_class, PROP_PROXY, param_spec);
Inconsistent style again.
+/*
+ * small helper function to help converting a
+ * telepathy dbus candidate to a list of FarsightTransportInfos
+ * nothing is copied, so always keep the usage of this within a function
+ * if you need to do multiple candidates, call this repeatedly and
+ * g_list_join them together.
+ * Free the list using free_fs_transports
+ */
This comment is not very clear. Perhaps proper capitalisation and punctuation
would help.
+ /* or S_CANDIDATE_TYPE_PRFLX .. if can't know */
What?
You sometimes indent lables to 0, 1, or 2 spaces. Pick one.
Consider using the code in other Telepathy components for automatically
generating signal marshaller list files.
Overall: code seems ok; it just needs some cleaning up.
--
Dafydd
More information about the Telepathy
mailing list