Created attachment 345953 [details] [review]
timecodestamper: Timecode from current system time

Created attachment 345976 [details] [review]
timecodestamper: Timecode from current system time

Review of attachment 345953 [details] [review]:

::: gst/timecode/gsttimecodestamper.c
@@ +142,3 @@
+      g_param_spec_boxed ("first-timecode",
+          "Timecode at the first frame",
+          "If unset, the timecode will start at 0",

You should write a description (and check that it works) about which property overrides which. For instance, it would make sense for one of FIRST_TIMECODE and FIRST_NOW to override the other, and for both to override CLOCK_SOURCE. Maybe you should add a "mode" option that would point to one of {running-time, from-clock, from-date-time, now}, but that would require the user to set two properties. Which solution do you think looks cleaner?

@@ +477,3 @@
     }
+  } else if (timecodestamper->source_clock == NULL &&
+      timecodestamper->first_tc == NULL) {

Add some sanity checks (especially in this function) that the timecodestamper won't act up in case e.g. both source-clock and first-timecode are given.

Review of attachment 345953 [details] [review]:

::: gst/timecode/gsttimecodestamper.c
@@ +142,3 @@
+      g_param_spec_boxed ("first-timecode",
+          "Timecode at the first frame",
+          "If unset, the timecode will start at 0",

You should write a description (and check that it works) about which property overrides which. For instance, it would make sense for one of FIRST_TIMECODE and FIRST_NOW to override the other, and for both to override CLOCK_SOURCE. Maybe you should add a "mode" option that would point to one of {running-time, from-clock, from-date-time, now}, but that would require the user to set two properties. Which solution do you think looks cleaner?

@@ +477,3 @@
     }
+  } else if (timecodestamper->source_clock == NULL &&
+      timecodestamper->first_tc == NULL) {

Add some sanity checks (especially in this function) that the timecodestamper won't act up in case e.g. both source-clock and first-timecode are given.

Created attachment 346357 [details] [review]
timecodestamper: Timecode from current system time

Review of attachment 346357 [details] [review]:

Looks good to me, thanks!

Review of attachment 346357 [details] [review]:

Looks good but please split this into 3 patches. These are 3 separate changes, even your commit message reads like that :)

::: gst/timecode/gsttimecodestamper.c
@@ +304,3 @@
+    timecodestamper->current_tc->minutes = timecodestamper->first_tc->minutes;
+    timecodestamper->current_tc->seconds = timecodestamper->first_tc->seconds;
+    timecodestamper->current_tc->frames = timecodestamper->first_tc->frames;

Should maybe be documented that only these fields are taken (what about field count btw?) and not flags, framerate, etc.

Created attachment 346514 [details] [review]
timecodestamper: First timecode property

Created attachment 346515 [details] [review]
timecodestamper: First timecode from current system time

Created attachment 346516 [details] [review]
timecodestamper: Remove clock-source property

Attachment 346514 [details] pushed as 290f3ca - timecodestamper: First timecode property
Attachment 346515 [details] pushed as 2cc6264 - timecodestamper: First timecode from current system time
Attachment 346516 [details] pushed as fc2ca69 - timecodestamper: Remove clock-source property

I wonder if "first-time-from-systemtime" or such would be better than "first-time-from-now"? I find the "now" part confusing. Does it refer to the moment when the property is set to TRUE? Or rather the moment the first buffer comes in?

The "now" was chosen based on the function it uses:
g_date_time_new_now_local ();

It is set at the first GST_EVENT_SEGMENT

Sure, but are you saying you don't see why it would be confusing? :)