After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 778702 - videotimecode: Init from GDateTime
videotimecode: Init from GDateTime
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal enhancement
: 1.11.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 778703 779010
 
 
Reported: 2017-02-15 18:02 UTC by Georg Lippitsch
Modified: 2017-02-23 17:53 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
videotimecode: Init from GDateTime (3.15 KB, patch)
2017-02-15 18:02 UTC, Georg Lippitsch
none Details | Review
videotimecode: Init from GDateTime (6.64 KB, patch)
2017-02-16 15:25 UTC, Georg Lippitsch
none Details | Review
videotimecode: Init from GDateTime (6.51 KB, patch)
2017-02-16 17:30 UTC, Georg Lippitsch
none Details | Review
videotimecode: Init from GDateTime (9.02 KB, patch)
2017-02-17 13:26 UTC, Georg Lippitsch
none Details | Review
videotimecode: Init from GDateTime (9.35 KB, patch)
2017-02-21 11:26 UTC, Georg Lippitsch
none Details | Review
videotimecode: Init from GDateTime (9.35 KB, patch)
2017-02-22 20:32 UTC, Georg Lippitsch
committed Details | Review

Description Georg Lippitsch 2017-02-15 18:02:20 UTC
Created attachment 345862 [details] [review]
videotimecode: Init from GDateTime

Add a function to init the time code from a GDateTime
Comment 1 Sebastian Dröge (slomo) 2017-02-15 18:07:03 UTC
Review of attachment 345862 [details] [review]:

::: gst-libs/gst/video/gstvideotimecode.c
@@ +202,3 @@
+ * midnight, and timecode is set to the given time.
+ *
+ * Returns: the #GVideoTimeCode representation of @dt.

Since: 1.12

Also add the function to docs/libs/*sections.txt and win32/common/libgstvideo.def (for the latter, just run "make update-exports" at the top-level)

You probably also want a _new_from_date_time()
Comment 2 Georg Lippitsch 2017-02-16 15:25:37 UTC
Created attachment 345950 [details] [review]
videotimecode: Init from GDateTime
Comment 3 Georg Lippitsch 2017-02-16 17:30:56 UTC
Created attachment 345974 [details] [review]
videotimecode: Init from GDateTime
Comment 4 Vivia Nikolaidou 2017-02-16 17:32:07 UTC
Review of attachment 345950 [details] [review]:

Looks good generally, but don't forget to add some unit tests as well!

::: gst-libs/gst/video/gstvideotimecode.c
@@ +226,3 @@
+      g_date_time_get_second (dt), frames, field_count);
+
+  gst_video_time_code_add_frames (tc, frames);

Why do you init using the frames and then re-add the frames?

You should also add a check in case the timecode is invalid, e.g. 01:02:03;00
Comment 5 Vivia Nikolaidou 2017-02-16 17:33:55 UTC
Review of attachment 345974 [details] [review]:

Looks good, I think it can be committed when a couple of unit tests are added to cover the new functionality. Thanks! :)

::: gst-libs/gst/video/gstvideotimecode.c
@@ +224,3 @@
+  gst_video_time_code_init (tc, fps_n, fps_d, jam, flags,
+      0, 0, 0, 0, field_count);
+  gst_video_time_code_add_frames (tc, frames);

Yes, that should work better than the previous one :)
Comment 6 Georg Lippitsch 2017-02-17 13:26:20 UTC
Created attachment 346070 [details] [review]
videotimecode: Init from GDateTime
Comment 7 Georg Lippitsch 2017-02-21 11:26:02 UTC
Created attachment 346313 [details] [review]
videotimecode: Init from GDateTime
Comment 8 Sebastian Dröge (slomo) 2017-02-22 18:49:56 UTC
Review of attachment 346313 [details] [review]:

::: gst-libs/gst/video/gstvideotimecode.c
@@ +229,3 @@
+  if (tc->config.flags & GST_VIDEO_TIME_CODE_FLAGS_DROP_FRAME) {
+    guint df = gst_util_uint64_scale_round (1,
+        tc->config.fps_n, tc->config.fps_d) / 15;

This looks weird. No reason to use gst_util_uint64_scale*() here as all arguments are plain ints and this is never going to overflow... and the / 15 could as well be multiple to the fps_d, no?

@@ +235,3 @@
+  }
+
+  g_return_if_fail (gst_video_time_code_is_valid (tc));

Why would this not be valid after initialized like this? Are there possible cases? And if so, would it be a programming error from the user (g_return_if_fail()), or an internal inconsistency (g_assert())?
Comment 9 Vivia Nikolaidou 2017-02-22 19:01:49 UTC
(In reply to Sebastian Dröge (slomo) from comment #8)
> Review of attachment 346313 [details] [review] [review]:
> 
> ::: gst-libs/gst/video/gstvideotimecode.c
> @@ +229,3 @@
> +  if (tc->config.flags & GST_VIDEO_TIME_CODE_FLAGS_DROP_FRAME) {
> +    guint df = gst_util_uint64_scale_round (1,
> +        tc->config.fps_n, tc->config.fps_d) / 15;
> 
> This looks weird. No reason to use gst_util_uint64_scale*() here as all
> arguments are plain ints and this is never going to overflow... and the / 15
> could as well be multiple to the fps_d, no?

fps_d will be 1001. I'm fairly sure that can't be divided by 15 :)

> @@ +235,3 @@
> +  }
> +
> +  g_return_if_fail (gst_video_time_code_is_valid (tc));
> 
> Why would this not be valid after initialized like this? Are there possible
> cases? And if so, would it be a programming error from the user
> (g_return_if_fail()), or an internal inconsistency (g_assert())?

Programming error from the user, so g_return_if_fail is correct.
Comment 10 Georg Lippitsch 2017-02-22 20:32:41 UTC
Created attachment 346496 [details] [review]
videotimecode: Init from GDateTime

gst_util_uint64_scale_round replaced.
Comment 11 Sebastian Dröge (slomo) 2017-02-23 17:51:06 UTC
Review of attachment 346496 [details] [review]:

::: gst-libs/gst/video/gstvideotimecode.c
@@ +215,3 @@
+
+  jam = g_date_time_new_local (g_date_time_get_year (dt),
+      g_date_time_get_month (dt), g_date_time_get_day_of_month (dt), 0, 0, 0.0);

This is leaked, fixed locally.
Comment 12 Sebastian Dröge (slomo) 2017-02-23 17:52:35 UTC
Attachment 346496 [details] pushed as b3df578 - videotimecode: Init from GDateTime