GNOME Bugzilla – Bug 778702
videotimecode: Init from GDateTime
Last modified: 2017-02-23 17:53:19 UTC
Created attachment 345862 [details] [review] videotimecode: Init from GDateTime Add a function to init the time code from a GDateTime
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()
Created attachment 345950 [details] [review] videotimecode: Init from GDateTime
Created attachment 345974 [details] [review] videotimecode: Init from GDateTime
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
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 :)
Created attachment 346070 [details] [review] videotimecode: Init from GDateTime
Created attachment 346313 [details] [review] videotimecode: Init from GDateTime
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())?
(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.
Created attachment 346496 [details] [review] videotimecode: Init from GDateTime gst_util_uint64_scale_round replaced.
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.
Attachment 346496 [details] pushed as b3df578 - videotimecode: Init from GDateTime