GNOME Bugzilla – Bug 772764
GstVideoTimeCode lacks GstValue functions
Last modified: 2017-01-09 16:56:59 UTC
GstVideoTimeCode has no registered string transform, compare or serialization/deserialization functions. The first would enable easier binding while the last would make the timecode-string property on timecodewait unnecessary.
Do you plan on making a patch for this?
No, I do not. It's just something I noticed while looking at the GVTC code during the conference.
Created attachment 342633 [details] [review] 0001-videotimecode-Add-GstValue-functions.patch Please see attached patch.
Comment on attachment 342633 [details] [review] 0001-videotimecode-Add-GstValue-functions.patch Nice, just one question: is all this GOnce stuff needed? I believe G_DEFINE_BOXED_TYPE does that internally already, and we can rely on the extra code being within the GOnce protected section.
Created attachment 342634 [details] [review] 0001-videotimecode-Add-GstValue-functions.patch Oops, you're right. :) Also fixed more issues.
Created attachment 342635 [details] [review] 0001-videotimecode-Add-GstValue-functions.patch (sorry for the noise, should be OK now)
Created attachment 342639 [details] [review] 0001-videotimecode-Added-GstValue-functions-unit-test.patch Added unit test.
Review of attachment 342635 [details] [review]: ::: gst-libs/gst/video/gstvideotimecode.c @@ +550,3 @@ + tc = gst_video_time_code_new_from_string (tc_str); + g_value_set_boxed (tc_val, tc); + gst_video_time_code_free (tc); g_value_take_boxed(), then you don't have a useless copy here and don't need to free @@ +560,3 @@ + + tc_str = gst_video_time_code_to_string (tc); + g_value_set_string (str_val, tc_str); g_value_take_string() (see above) @@ +579,3 @@ + return FALSE; + + g_value_set_boxed (dest, tc); g_value_take_boxed() @@ +601,3 @@ + gchar **parts; + + parts = g_strsplit (tc_str, ":", 4); g_strsplit_set(tc_str, ":;,", 4) so that it also works for all the other variants. But that needs validation that the string is not e.g. "12;32,23" or something like that. You could do if (sscanf (tc_str, "%02u:%02u:%02u:%02u", &hours, &minutes, &seconds, &frames) == 4 || sscanf (tc_str, "%02u:%02u:%02u;%02u", &hours, &minutes, &seconds, &frames) == 4 || sscanf (tc_str, "%02u:%02u:%02u.%02u", &hours, &minutes, &seconds, &frames) == 4 || sscanf (tc_str, "%02u:%02u:%02u,%02u", &hours, &minutes, &seconds, &frames) == 4) { // good case } else { // error }
Created attachment 343168 [details] [review] 0001-videotimecode-Add-GstValue-functions.patch
commit 5cbb52285c40c74bcdfcbd283d2d1b2b1634ed8f Author: Vivia Nikolaidou <vivia@ahiru.eu> Date: Fri Dec 30 20:27:48 2016 +0200 videotimecode: Add GstValue functions unit test https://bugzilla.gnome.org/show_bug.cgi?id=772764 commit 06ff74e51dd83299f62dfc176804c57bd2f3a704 Author: Vivia Nikolaidou <vivia@toolsonair.com> Date: Fri Dec 30 19:08:16 2016 +0200 videotimecode: Add GstValue functions Add compare, serialization and deserialization functions https://bugzilla.gnome.org/show_bug.cgi?id=772764