GNOME Bugzilla – Bug 767950
qtmux: Add support for writing timecode track
Last modified: 2016-08-17 06:05:47 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 .
This is based on https://developer.apple.com/library/mac/documentation/QuickTime/QTFF/QTFFChap5/qtff5.html#//apple_ref/doc/uid/TP40000939-CH207-19311 btw
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.
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.
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.
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.
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?
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.
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?
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.
Review of attachment 333053 [details] [review]: Thanks for the updates, looks good.
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