GNOME Bugzilla – Bug 599729
Show timer when recording
Last modified: 2012-03-13 16:50:08 UTC
"It will be really nice to see how long the video is recording. Use case: - you need to make some reportage for vlog, and need to know the record time to make thing shorter or longer." From: https://bugs.edge.launchpad.net/ubuntu/+source/cheese/+bug/457994
videomixer and cairotimeoverlay should be able to do this.
Created attachment 209276 [details] [review] Show a timer when recording a video. This patch solves the bug.
Review of attachment 209276 [details] [review]: When I test this, I get lots of warnings on the console and no countdown is shown: (cheese:13826): Clutter-CRITICAL **: clutter_container_add_actor: assertion `CLUTTER_IS_ACTOR (actor)' failed (cheese:13826): Clutter-CRITICAL **: clutter_actor_set_position: assertion `CLUTTER_IS_ACTOR (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_actor_raise: assertion `CLUTTER_IS_ACTOR (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_actor_hide: assertion `CLUTTER_IS_ACTOR (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_actor_set_position: assertion `CLUTTER_IS_ACTOR (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_actor_raise: assertion `CLUTTER_IS_ACTOR (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_actor_hide: assertion `CLUTTER_IS_ACTOR (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_actor_hide: assertion `CLUTTER_IS_ACTOR (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_text_set_text: assertion `CLUTTER_IS_TEXT (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_actor_show: assertion `CLUTTER_IS_ACTOR (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_actor_set_position: assertion `CLUTTER_IS_ACTOR (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_actor_raise: assertion `CLUTTER_IS_ACTOR (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_actor_set_position: assertion `CLUTTER_IS_ACTOR (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_actor_raise: assertion `CLUTTER_IS_ACTOR (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_text_set_text: assertion `CLUTTER_IS_TEXT (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_text_set_text: assertion `CLUTTER_IS_TEXT (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_text_set_text: assertion `CLUTTER_IS_TEXT (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_text_set_text: assertion `CLUTTER_IS_TEXT (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_text_set_text: assertion `CLUTTER_IS_TEXT (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_text_set_text: assertion `CLUTTER_IS_TEXT (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_text_set_text: assertion `CLUTTER_IS_TEXT (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_text_set_text: assertion `CLUTTER_IS_TEXT (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_text_set_text: assertion `CLUTTER_IS_TEXT (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_text_set_text: assertion `CLUTTER_IS_TEXT (self)' failed (cheese:13826): Clutter-CRITICAL **: clutter_text_set_text: assertion `CLUTTER_IS_TEXT (self)' failed I guess that there was a problem constructing the ClutterText. Backtrace with G_DEBUG=fatal-criticals: Clutter-CRITICAL **: clutter_container_add_actor: assertion `CLUTTER_IS_ACTOR (actor)' failed Program received signal SIGTRAP, Trace/breakpoint trap. g_logv (log_domain=0x7ffff4064078 "Clutter", log_level=<optimized out>, format=0x7fffeebf6b4a "%s: assertion `%s' failed", args1=0x7fffffffd4d0) at gmessages.c:577 577 gmessages.c: No such file or directory. in gmessages.c (gdb) bt
+ Trace 229840
4831 _tmp154_ = self->priv->viewport; 4832 _tmp155_ = self->priv->background_layer; 4833 clutter_container_add_actor ((ClutterContainer*) _tmp154_, (ClutterActor*) _tmp155_); 4834 _tmp156_ = self->priv->viewport; 4835 _tmp157_ = self->priv->timeout_layer; 4836 clutter_container_add_actor ((ClutterContainer*) _tmp156_, (ClutterActor*) _tmp157_); 4837 _tmp158_ = self->priv->viewport_layout; 4838 _tmp159_ = self->priv->viewport_layout_manager; 4839 clutter_box_set_layout_manager (_tmp158_, (ClutterLayoutManager*) _tmp159_); 4840 _tmp160_ = self->priv->viewport; (gdb) print self->priv->timeout_layer $1 = (ClutterText *) 0x0 (gdb) print self->priv->error_layer $2 = (ClutterText *) 0xbf0c00 As error_layer in not NULL, it seems that there needs to be some extra checking. ::: libcheese/cheese-camera.c @@ +1035,3 @@ + // Necessary to reset the base and clock times for each + // new recording. + gst_element_set_state (priv->camerabin, GST_STATE_NULL); This works the first time, but does not seem to work for subsequent video recordings, which produce files in which GStreamer is unable to find a video stream. @@ +1735,3 @@ + * + * Retrieve the already recorded time, until this function + * is call, of the currently video that is being recorded. This does not make sense. It should read ‘Get a string representation of the playing time of the current video recording.’ @@ +1737,3 @@ + * is call, of the currently video that is being recorded. + * + * Returns: A const gchar * with the recorded time. If you return a pointer to a const char, you say that the returned value should not be modified, whereas the string was allocated on the heap by g_strdup_printf(), which returns a pointer to a char, not a pointer to a const char. @@ +1742,3 @@ +cheese_camera_get_recorded_time (CheeseCamera *camera) +{ + CheeseCameraPrivate *priv = camera->priv; Before dereferencing the camera pointer, you should check that it is valid. @@ +1743,3 @@ +{ + CheeseCameraPrivate *priv = camera->priv; + GstClock *clock = NULL; There is no point initialising this and the following variables to NULL or 0, so do not. @@ +1748,3 @@ + GstClockTime times_diff = 0; + GDateTime *total_time = NULL; + int hours = 0; Use the gint typedef. @@ +1751,3 @@ + int minutes = 0; + int seconds = 0; + const gchar *timer; As this is not a timer nor a GTimer, it should be called ‘time_string’ or similar. @@ +1757,3 @@ + clock_time = gst_clock_get_time (clock); + + times_diff = clock_time + base_time; Why are the times added together, when you want the difference? The gst_element_base_time() documentation says ‘Subtracting the base time from the clock time gives the running time of the element.’. @@ +1758,3 @@ + + times_diff = clock_time + base_time; + total_time = g_date_time_new_from_unix_utc (times_diff/1000000000); This needs a comment, and the 1000000000 should be replaced with a constant. @@ +1762,3 @@ + minutes = g_date_time_get_minute (total_time); + seconds = g_date_time_get_second (total_time); + timer = g_strdup_printf ("%02i:%02i:%02i", You should be using g_date_time_format() rather than building up the string in this way. Frankly, building up a GDateTime like this is silly, and it would be better to just do what Totem does: http://git.gnome.org/browse/totem/tree/src/backend/video-utils.c#n63 That way has the added benefit of being translatable. @@ +1765,3 @@ + hours, minutes, seconds); + + g_object_unref (clock); Should be gst_object_unref(). ::: src/cheese-window.vala @@ +1043,3 @@ { camera.stop_video_recording (); + timeout_layer.text = "00:00:00"; The countdown should also be hidden here, as otherwise I guess that it is visible even after the stream is stopped. @@ +1054,3 @@ /** + * Update the timeout layer displayed timer. + */ Needs a comment about why this returns true or false.
Created attachment 209329 [details] [review] Updated patch based on review in comment https://bugzilla.gnome.org/show_bug.cgi?id=599729#c3 As already spoken with Dave: * I could not reproduce the first problem he mentioned regarding the Clutter-CRITICAL's in the terminal. I have tried it several times but everything worked as expected. * Regarding the following point: " ::: libcheese/cheese-camera.c @@ +1035,3 @@ + // Necessary to reset the base and clock times for each + // new recording. + gst_element_set_state (priv->camerabin, GST_STATE_NULL); This works the first time, but does not seem to work for subsequent video recordings, which produce files in which GStreamer is unable to find a video stream. " This is a bug in master, and I just filed a bug (Bug 671725) about it. * All the other points have been modified or I have added comments to them in order to explain the reasons why they were made the way they are.
Review of attachment 209329 [details] [review]: ::: libcheese/cheese-camera.c @@ +1733,3 @@ + * of the current video recording + * + * Returns: A gchar * with the time representation. Just say that it returns a string, as the type is already given in the function signature. @@ +1738,3 @@ +cheese_camera_get_recorded_time (CheeseCamera *camera) +{ + g_assert (camera); This does not check if the camera is valid, just that it is not NULL. I am quite disappointed that you did not manage to look at other exported symbols in cheese-camera.c to figure out the correct approach. @@ +1741,3 @@ + + CheeseCameraPrivate *priv = camera->priv; + const NSEC_TO_SEC = 1000000000; You should have seen the warnings here, but you forgot to give a type. @@ +1755,3 @@ + clock = gst_element_get_clock (priv->camerabin); + clock_time = gst_clock_get_time (clock); + // In order to calculate the running time of the stream Use the /* */ (C89) comment style in C code. @@ +1761,3 @@ + + // Substract seconds, minutes and hours. + total_time = time_diff / NSEC_TO_SEC; Use GST_TIME_AS_SECONDS to do the conversion. @@ +1768,3 @@ + hours = total_time / (TUNIT_60 * TUNIT_60); + + gst_object_unref (clock); Move the unref to just after when you used the clock. @@ +1770,3 @@ + gst_object_unref (clock); + + return g_strdup_printf (C_("time format", "%02i:%02i:%02i"), There should be a translator comment here, like the one in Totem. ::: src/cheese-window.vala @@ +1034,3 @@ camera.start_video_recording (fileutil.get_new_media_filename (this.current_mode)); + // Will be called every 1 second while update_timeout_layer returns true. + Timeout.add (1000, update_timeout_layer); Might be better to use Timeout.add_seconds(), as the inaccuracy is not a problem. @@ +1045,3 @@ camera.stop_video_recording (); + // The timeout_layer always shows the "00:00:00" + // string when not recording, in order to notify Use /* */ comment style for multi-line comments.
Created attachment 209346 [details] [review] Updated patch based on review in comment https://bugzilla.gnome.org/show_bug.cgi?id=599729#c5
Comment on attachment 209346 [details] [review] Updated patch based on review in comment https://bugzilla.gnome.org/show_bug.cgi?id=599729#c5 Looks good. Once the problem with the NULL ClutterText is figured out, this can be pushed to the cheese-next branch.
Created attachment 209505 [details] [review] Updated with small change I just realized that I did not need to raise the timer layer to the top to be shown, but just add it to the stage in the last place. This patch also includes this change.
Created attachment 209541 [details] [review] Updated patch based on review in comment https://bugzilla.gnome.org/show_bug.cgi?id=599729#c7 The problem with the 'Clutter-Critical' messages has been solved. The patch was working well on my system, but not on Dave's one, who discovered, that the problem was the Pango font description. It seems to be that it does not work the same for all the systems, but we have solved it now by changing the order of the attributes for the font description. The patch has been merged already: 03222e034b0a7cfe119375f3bc0c8f397c3352e3.
Hmm... i just compiled cheese with your patch and i get this "Clutter-Critical" error. And there is not text overlay :(
Hello Oleksij, that is expected. The last changes are in the repository. As I said in my last comment, the changes have been pushed there (cheese-next branch). You can see them in the following link: http://git.gnome.org/browse/cheese/commit/?h=cheese-next&id=03222e034b0a7cfe119375f3bc0c8f397c3352e3 Please, let us know if you still have any problem! :)
Patricia, thank you for your response. I already discussed it with David on irc and found the reason of this errors. (i use cheese-next.git) The reason is the wrapperscript in source repository. I usually do not install cheese, just compile and run it with this wrapper. But json file will be not included for some reasons. So i was using old json with new source.. that is the reason for this errors.