GNOME Bugzilla – Bug 604869
Add timeout in freeze element
Last modified: 2011-09-22 09:16:44 UTC
Created attachment 149949 [details] [review] Add timeout property to freeze The attached patch adds a property to the freeze element to end after a timeout o 'x' seconds occurs. This comes in handy when prototyping decoding of still images.
Review of attachment 149949 [details] [review]: ::: gst/freeze/gstfreeze.c @@ +45,3 @@ ARG_0, ARG_MAX_BUFFERS, + ARG_TIMEOUT, No trailing comma for the last enum please (some compilers don't like that) @@ +55,3 @@ "Gergely Nagy <gergely.nagy@neteyes.hu>," + "Renato Filho <renato.filho@indt.org.br>," + "Daniel Diaz <yosoy@danieldiaz.org>"); Please don't add your name here for trivial patches, feel free to add a Copyright line though. @@ +126,3 @@ + g_param_spec_int ("timeout", + "timeout", + "Timeout before closing stream", 0, G_MAXINT, 1, G_PARAM_READWRITE)); You should mention the units of the timeout here and that 0 = no timeout. @@ +403,3 @@ + + gst_element_get_state (GST_ELEMENT (freeze), &cur_state, NULL, 0); + if (cur_state != GST_STATE_PLAYING) This does not really seem right conceptually. A filter should behave the same in paused and playing state really. Also, you don't take account of intermediate pause states and the like (not a big deal, just saying). @@ +406,3 @@ + return TRUE; + + gst_pad_push_event (freeze->srcpad, gst_event_new_eos ()); This is not really entirely correct. At the very least the EOS should be sent from the streaming thread. @@ +416,3 @@ + + if (freeze->timeout > 0) + g_timeout_add (freeze->timeout * 1000, gst_freeze_finish_stream, freeze); You can't/shouldn't rely on the default GLib main context being iterated in an element.
Is there still interest in this patch? There's also the imagefreeze element in gst-plugins-good now, which does the same as freeze but better ;)
(In reply to comment #2) > Is there still interest in this patch? There's also the imagefreeze element in > gst-plugins-good now, which does the same as freeze but better ;) Cool! Still, imagefreeze doesn't finish the stream by itself, which was the purpose of this patch. This, again, is just for quick prototyping, as in acquiring an image from a video sensor, see it for some seconds, and be done with it. Can also be used with quick image decoding or cases like that.
Created attachment 188464 [details] [review] Add timeout property to freeze, take 2 Address feedback (thanks!) from Tim, rebase to 0.10.22.
imagefreeze ends the stream by itself if you send a SEEK event to it with a stop position
You didn't address the g_timeout_add() comment though, this definitely has to be fixed before this patch can be accepted.
Shouldn't the timeout be "stream time" based, ie. "stop after pushing out X seconds worth of frames", rather than "stop after X seconds have passed according to the clock on the wall"?
Daniel, could you please provide reworked patch including comment#6 and comment#7 ?
(In reply to comment #5) > imagefreeze ends the stream by itself if you send a SEEK event to it with a > stop position Cool, but for gst-launch it still needs to terminate somehow, which means human interaction. The original purpose of this patch, in the spirit of gst-launch's quick prototyping, was to enable a quick tool to visualize acquired images. If imagefreeze works with gst-auto-launch (which can request state transitions on timed stages), I'll definitely use that instead!
(In reply to comment #8) > Daniel, could you please provide reworked patch including comment#6 and > comment#7 ? Appy-polly-logies, but I don't think I'll have that anytime soon. It's not even on my to-do list anymore. Please close this bug or let somebody else work on it if there's still any interest on it.