[Bug 767950] qtmux: Add support for writing timecode track

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=767950

Thiago Sousa Santos <thiagossantos at gmail.com> changed:

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

--- Comment #8 from Thiago Sousa Santos <thiagossantos at gmail.com> ---
Review of attachment 332953:
 --> (https://bugzilla.gnome.org/review?bug=767950&attachment=332953)

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?

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.