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 767950 - qtmux: Add support for writing timecode track
qtmux: Add support for writing timecode track
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: 1.9.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 766419
Blocks:
 
 
Reported: 2016-06-22 14:42 UTC by Vivia Nikolaidou
Modified: 2016-08-17 06:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Preliminary patch file for atoms.c and friends (13.74 KB, patch)
2016-06-22 14:42 UTC, Vivia Nikolaidou
none Details | Review
0001-qtmux-Added-support-for-writing-timecode-track.patch (23.28 KB, patch)
2016-06-24 13:35 UTC, Vivia Nikolaidou
none Details | Review
0001-qtmux-Added-support-for-writing-timecode-track.patch (23.48 KB, patch)
2016-06-28 16:55 UTC, Vivia Nikolaidou
none Details | Review
0001-qtmux-Added-support-for-writing-timecode-track.patch (26.60 KB, patch)
2016-07-11 09:21 UTC, Vivia Nikolaidou
none Details | Review
0001-qtmux-Added-support-for-writing-timecode-track.patch (27.18 KB, patch)
2016-08-08 20:19 UTC, Vivia Nikolaidou
none Details | Review
0001-qtmux-Added-support-for-writing-timecode-track.patch (27.65 KB, patch)
2016-08-10 09:41 UTC, Vivia Nikolaidou
committed Details | Review

Description Vivia Nikolaidou 2016-06-22 14:42:15 UTC
Created attachment 330196 [details] [review]
Preliminary patch file for atoms.c and friends

qtmux should be able to write the timecode track, if it is contained in the video frames.

Attaching a preliminary patch for atoms.c and friends. I'd appreciate any feedback on this while I move on to gstqtmux.c/h .
Comment 2 Thiago Sousa Santos 2016-06-24 02:53:20 UTC
Review of attachment 330196 [details] [review]:

Looks overall good, just one minor issue found.

::: gst/isomp4/atoms.c
@@ +2115,3 @@
+    prop_copy_uint8 (tmcd->name.string_length, buffer, size, offset);
+    prop_copy_uint8 (tmcd->name.language_code, buffer, size, offset);
+    prop_copy_fixed_size_string ((guint8 *)tmcd->name.name, tmcd->name.string_length, buffer, size, offset);

You would need to also write this atom's size here.
Comment 3 Vivia Nikolaidou 2016-06-24 13:35:18 UTC
Created attachment 330319 [details] [review]
0001-qtmux-Added-support-for-writing-timecode-track.patch

Thanks for your comments. This patch should hopefully be finished, apart from a FIXME comment in atoms.c (but I see other similar cases elsewhere, so maybe it can be left as it is).

Note that it requires the patches from
https://bugzilla.gnome.org/show_bug.cgi?id=766419
in order to work.
Comment 4 Vivia Nikolaidou 2016-06-28 16:55:49 UTC
Created attachment 330498 [details] [review]
0001-qtmux-Added-support-for-writing-timecode-track.patch

Found an issue with the previous patch: If the first frame that gets into the muxer isn't the first one to be displayed (B-frame), the timecode would be off. Fixed it now using the buffer PTS and DTS.
Comment 5 Vivia Nikolaidou 2016-07-11 09:21:57 UTC
Created attachment 331194 [details] [review]
0001-qtmux-Added-support-for-writing-timecode-track.patch

My previous approach turned out to not work as expected. This has been tested and found to work well so fa: wait until the frame whose DTS is higher than the PTS of the first frame ever received, and compare timestamps until then.
Comment 6 Thiago Sousa Santos 2016-07-25 03:06:28 UTC
Review of attachment 331194 [details] [review]:

::: gst/isomp4/atoms.c
@@ +1296,3 @@
+{
+  atom_tref_clear (tref);
+

atom_tref_clear already calls atom_clear.

@@ +2595,3 @@
+
+  if (atom_array_get_len (&tref->entries) == 0) {
+    /* FIXME not needing this atom might be confused with error while copying */

Maybe it should be checked before called. We could also rework these copy_data to have different return but let's keep that to later.

@@ +2605,3 @@
+  /* FIXME where does 8 + 4 * length come from? */
+  prop_copy_uint32 (8 + 4 * atom_array_get_len (&tref->entries), buffer, size,
+    return 0;

8 is from the fourcc + size  (this size that is written here and the next foucc copy) and 4 is the size of the array entry.

::: gst/isomp4/atoms.h
@@ +533,3 @@
+
+  guint32 reftype;
+  //guint32 tref_index;

Replace with /* comment */

::: gst/isomp4/gstqtmux.c
@@ +634,3 @@
       DEFAULT_RESERVED_BYTES_PER_SEC_PER_TRAK;
+  qtmux->first_pts = GST_CLOCK_TIME_NONE;
+  qtmux->tc_pos = -1;

Maybe move this to _reset()

@@ +3125,3 @@
+           * we've already received the first frame to be presented. Otherwise
+           * the decoder would need to go back in time */
+          frames_since_daily_jam = GUINT32_TO_BE (frames_since_daily_jam);

When this happens, what brings back the writing position to the latest one to continue writing data correctly?
Comment 7 Vivia Nikolaidou 2016-08-08 20:19:20 UTC
Created attachment 332953 [details] [review]
0001-qtmux-Added-support-for-writing-timecode-track.patch

Found a small issue - setting state to READY and back to PAUSED should reset any timecode information. Fixed.
Comment 8 Thiago Sousa Santos 2016-08-10 03:29:53 UTC
Review of attachment 332953 [details] [review]:

This is looking good, only some minor stuff to fix before merge.

Thanks for working on this!

::: gst/isomp4/atoms.c
@@ +421,3 @@
+  tcmi->bg_color[2] = 0;
+  if (tcmi->font_name) {
+  atom_full_clear (&tcmi->header);

g_free is NULL-safe already, no need for this 'if'

@@ +557,3 @@
+  tmcd->name.language_code = 0;
+  if (tmcd->name.name) {
+  tmcd->timescale = 0;

same comment about g_free and if here.

@@ +2603,3 @@
+  }
+
+    /* FIXME not needing this atom might be confused with error while copying */

This is this size + fourcc (8) plus 4 for each entry

::: gst/isomp4/atoms.h
@@ +290,3 @@
+  guint16 text_face;
+  guint16 text_size;
+  guint16 padding;

I think in the rest of the code we don't represent entries that have a fixed value. Just write them directly in the 'copy_data' functions. You can live a comment if you want, to document that there are padding/reserved bytes.

Not a minor issue but better to keep the code consistent.

@@ +310,3 @@
+  guint16 opcolor[3];
+  guint8 balance;
+typedef struct _AtomTMCD

Same here

@@ +533,3 @@
+
+  guint32 reftype;
+  //guint32 tref_index;

Change comment to /* */

::: gst/isomp4/gstqtmux.c
@@ +2528,3 @@
+  gst_segment_init (&segment, GST_FORMAT_BYTES);
+  segment.start = offset;
+  guint64 offset = qtmux->tc_pos;

I don't see where this is reset back to the original writing position. So when this is called it seems like it will start overwriting data after the timecode is updated.

@@ +3067,3 @@
   }
 
+  if (buf != NULL && (pad->tc_trak == NULL || qtmux->tc_pos != -1)) {

It would be nice to give this whole block its own function so we can name what this all is doing. Something like: gst_qt_mux_check_and_update_timecode()

What do you think?
Comment 9 Vivia Nikolaidou 2016-08-10 09:41:09 UTC
Created attachment 333053 [details] [review]
0001-qtmux-Added-support-for-writing-timecode-track.patch

Thanks for your review comments! Please review again this patch and let me know.
Comment 10 Thiago Sousa Santos 2016-08-17 02:27:08 UTC
Review of attachment 333053 [details] [review]:

Thanks for the updates, looks good.
Comment 11 Sebastian Dröge (slomo) 2016-08-17 06:05:25 UTC
commit cdb764990965d756813ce25695bb0e1974466e0c
Author: Vivia Nikolaidou <vivia@toolsonair.com>
Date:   Fri Jun 24 16:32:37 2016 +0300

    qtmux: Added support for writing timecode track
    
    https://bugzilla.gnome.org/show_bug.cgi?id=767950