Discussion:
[Bug 778702] New: videotimecode: Init from GDateTime
Add Reply
"GStreamer" (GNOME Bugzilla)
2017-02-15 18:02:20 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778702

Bug ID: 778702
Summary: videotimecode: Init from GDateTime
Classification: Platform
Product: GStreamer
Version: git master
OS: Linux
Status: NEW
Severity: normal
Priority: Normal
Component: gst-plugins-base
Assignee: gstreamer-***@lists.freedesktop.org
Reporter: ***@toolsonair.com
QA Contact: gstreamer-***@lists.freedesktop.org
GNOME version: ---

Created attachment 345862
--> https://bugzilla.gnome.org/attachment.cgi?id=345862&action=edit
videotimecode: Init from GDateTime

Add a function to init the time code from a GDateTime
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-15 18:05:22 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778702

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@coaxion.net
Severity|normal |enhancement
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-15 18:07:03 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778702

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #345862|none |needs-work
status| |

--- Comment #1 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Review of attachment 345862:
--> (https://bugzilla.gnome.org/review?bug=778702&attachment=345862)

::: 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()
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-15 18:09:49 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778702

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Blocks| |778703
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-16 15:25:37 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778702

Georg Lippitsch <***@toolsonair.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #345862|needs-work |none
status| |
Attachment #345862|0 |1
is obsolete| |

--- Comment #2 from Georg Lippitsch <***@toolsonair.com> ---
Created attachment 345950
--> https://bugzilla.gnome.org/attachment.cgi?id=345950&action=edit
videotimecode: Init from GDateTime
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-16 17:30:56 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778702

--- Comment #3 from Georg Lippitsch <***@toolsonair.com> ---
Created attachment 345974
--> https://bugzilla.gnome.org/attachment.cgi?id=345974&action=edit
videotimecode: Init from GDateTime
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-16 17:32:07 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778702

--- Comment #4 from Vivia Nikolaidou <***@ahiru.eu> ---
Review of attachment 345950:
--> (https://bugzilla.gnome.org/review?bug=778702&attachment=345950)

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
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-16 17:33:32 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778702

Georg Lippitsch <***@toolsonair.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #345950|0 |1
is obsolete| |
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-16 17:33:55 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778702

Vivia Nikolaidou <***@ahiru.eu> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #345974|none |needs-work
status| |

--- Comment #5 from Vivia Nikolaidou <***@ahiru.eu> ---
Review of attachment 345974:
--> (https://bugzilla.gnome.org/review?bug=778702&attachment=345974)

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 :)
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-17 13:26:20 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778702

Georg Lippitsch <***@toolsonair.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #345974|needs-work |none
status| |
Attachment #345974|0 |1
is obsolete| |

--- Comment #6 from Georg Lippitsch <***@toolsonair.com> ---
Created attachment 346070
--> https://bugzilla.gnome.org/attachment.cgi?id=346070&action=edit
videotimecode: Init from GDateTime
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-21 11:06:18 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778702

Georg Lippitsch <***@toolsonair.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Blocks| |779010
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-21 11:26:02 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778702

Georg Lippitsch <***@toolsonair.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346070|0 |1
is obsolete| |

--- Comment #7 from Georg Lippitsch <***@toolsonair.com> ---
Created attachment 346313
--> https://bugzilla.gnome.org/attachment.cgi?id=346313&action=edit
videotimecode: Init from GDateTime
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-22 18:49:56 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778702

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346313|none |reviewed
status| |

--- Comment #8 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Review of attachment 346313:
--> (https://bugzilla.gnome.org/review?bug=778702&attachment=346313)

::: 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())?
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-22 19:01:49 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778702

Vivia Nikolaidou <***@ahiru.eu> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@ahiru.eu

--- Comment #9 from Vivia Nikolaidou <***@ahiru.eu> ---
(In reply to Sebastian Dröge (slomo) from comment #8)
Post by "GStreamer" (GNOME Bugzilla)
::: 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 :)
Post by "GStreamer" (GNOME Bugzilla)
@@ +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.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-22 20:32:41 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778702

Georg Lippitsch <***@toolsonair.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346313|reviewed |none
status| |
Attachment #346313|0 |1
is obsolete| |

--- Comment #10 from Georg Lippitsch <***@toolsonair.com> ---
Created attachment 346496
--> https://bugzilla.gnome.org/attachment.cgi?id=346496&action=edit
videotimecode: Init from GDateTime

gst_util_uint64_scale_round replaced.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-23 17:51:06 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778702

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346496|none |reviewed
status| |

--- Comment #11 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Review of attachment 346496:
--> (https://bugzilla.gnome.org/review?bug=778702&attachment=346496)

::: 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.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-23 17:52:42 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778702

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346496|reviewed |committed
status| |
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-23 17:52:35 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778702

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED

--- Comment #12 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Attachment 346496 pushed as b3df578 - videotimecode: Init from GDateTime
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-23 17:53:19 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778702

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Target Milestone|git master |1.11.2
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
Loading...