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 792680 - qtmux: Make sure timecode uses the same timescale as video
qtmux: Make sure timecode uses the same timescale as video
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other All
: Normal normal
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 792649
Blocks:
 
 
Reported: 2018-01-19 13:09 UTC by Vivia Nikolaidou
Modified: 2018-01-23 14:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
qtmux: Make sure timecode uses the same timescale as video (5.26 KB, patch)
2018-01-19 13:10 UTC, Vivia Nikolaidou
none Details | Review
qtmux: Make sure timecode uses the same timescale as video (4.59 KB, patch)
2018-01-23 13:52 UTC, Vivia Nikolaidou
none Details | Review
qtmux: Make sure timecode uses the same timescale as video (4.55 KB, patch)
2018-01-23 14:20 UTC, Vivia Nikolaidou
committed Details | Review

Description Vivia Nikolaidou 2018-01-19 13:09:58 UTC
See commit message
Comment 1 Vivia Nikolaidou 2018-01-19 13:10:03 UTC
Created attachment 367079 [details] [review]
qtmux: Make sure timecode uses the same timescale as video

Don't blindly derive it from the frame rate, but try to get the per-pad
configured timescale first (if it exists)
Comment 2 Sebastian Dröge (slomo) 2018-01-19 14:45:38 UTC
Review of attachment 367079 [details] [review]:

::: gst/isomp4/gstqtmux.c
@@ +2591,3 @@
 
+        atom_trak_set_timecode_type (qpad->tc_trak, qtmux->context,
+            gst_qt_mux_pad_get_timescale (qtpad), tc);

You should take the actually configured timescale here from qpad->trak_ste->mdia.mdhd.time_info.timescale or so.

It might be configured via the pad property, via the muxer property, or taken from the framerate. That decision was done already and stored in the qpad.

@@ +4092,3 @@
 
+    atom_trak_set_timecode_type (pad->tc_trak, qtmux->context,
+        gst_qt_mux_pad_get_timescale (qtpad), pad->first_tc);

Same here
Comment 3 Vivia Nikolaidou 2018-01-23 13:52:04 UTC
Created attachment 367310 [details] [review]
qtmux: Make sure timecode uses the same timescale as video

Don't blindly derive it from the frame rate, but try to get the per-pad
configured timescale first (if it exists)
Comment 4 Vivia Nikolaidou 2018-01-23 13:53:11 UTC
Thanks, fixed. There was also another issue: frame duration seemed to be hardcoded to 100 for the timecode track. Fixed that too.
Comment 5 Sebastian Dröge (slomo) 2018-01-23 14:07:15 UTC
Review of attachment 367310 [details] [review]:

Looks good except for one thing

::: gst/isomp4/atoms.c
@@ +3825,3 @@
   trak->mdia.mdhd.time_info.timescale =
+      trak_timescale !=
+      0 ? trak_timescale : atom_framerate_to_timescale (tc->config.fps_n,

This can never be 0, can it?

@@ +3837,3 @@
   tmcd->timescale =
+      trak_timescale !=
+      0 ? trak_timescale : atom_framerate_to_timescale (tc->config.fps_n,

This can never be 0, can it?

@@ +3839,3 @@
+      0 ? trak_timescale : atom_framerate_to_timescale (tc->config.fps_n,
+      tc->config.fps_d);
+  tmcd->frame_duration = tmcd->timescale * tc->config.fps_d / tc->config.fps_n;

You probably want to use gst_util_uint64_scale_int() here, just in case timescale and fps_d are both rather big.
Comment 6 Vivia Nikolaidou 2018-01-23 14:20:04 UTC
Created attachment 367315 [details] [review]
qtmux: Make sure timecode uses the same timescale as video

Don't blindly derive it from the frame rate, but try to get the per-pad
configured timescale first (if it exists)
Comment 7 Sebastian Dröge (slomo) 2018-01-23 14:42:35 UTC
Attachment 367315 [details] pushed as 379059b - qtmux: Make sure timecode uses the same timescale as video