Re: [PULL] cheese: overrun callback for slow machines

[Filippo Argiolas <fargiolas gnome org>]

On Tue, Aug 24, 2010 at 3:07 AM, Brandon Philips <brandon ifup org> wrote:
> Hello-
>
> This is one half of my solution to slow netbooks running cheese and
> trying to encode videos[1]. The basic idea here is to detect a pipeline
> stall and inform the user that their machine is too slow to encode video
> in real time. It suggests that lowering the resolution might help.

I'm finally reviewing this stuff, sorry for the long delay.

> Brandon Philips (5):
>      fileutil: remove redundant code

This looks fine and can be applied as is.
For what is worth, please, in the future submit downstream patches in
bugzilla with a dedicated bug. This patch has nothing to do with the
overrun ones.

>      camera: move signals array into CheeseCameraPrivate

I commented this in bugzilla. This commit was meant to solve signal
emission issue from gstreamer threads, this can also be solved
emitting signals by name. Given it is a quite common practice to have
signals array as a global static guint[] I'll keep it that way for
now.

>      window: move info bar from no-camera to window
>      window: use fileutil's CheeseMediaMode

These don't apply in master. We'll need to think another UI for this
(this obviously doesn't prevent us to apply the cheese-camera bits
anyway).

>      camera: provide an overrun signal and implement callback

Would you please reply to a few concerns I have?

+void cheese_camera_pause (CheeseCamera *camera);

Maybe this should be removed?

+  gst_element_set_state (old_sink, GST_STATE_NULL);

Is this needed? cheese_camera_stop sets the pipeline old_sink belongs
to to NULL, so this should be redundant, isn't it?

+      if (!priv->overrun)
+        g_signal_emit (camera, priv->signals[CHEESE_SIGNAL_VIDEO_SAVED], 0);

What's the purpose of priv->overrun? as far as I can tell is either
undefined (probably 0 if the compiler assigns 0 to unassigned ints) or
it's assigned to 0 in cheese_camera_play. Am I missing anything?

+    g_object_set (save_queue, "max-size-time",
G_GINT64_CONSTANT(5000000000), NULL);

Is there any rationale behind the choice of this value?


Thanks again for the patches,

Filippo