GNOME Bugzilla – Bug 696437
qtmux: add support for mp4 timed text
Last modified: 2014-09-28 12:04:34 UTC
First patch changes the way sample delta is calculated. The variable is called duration, but it is used as timestamp delta for samples so it probably should be calculated as dts (or pts if no dts are present) delta instead from duration. Otherwise sparse streams and streams with discontinuities will have wrong timestamps.
Created attachment 239607 [details] [review] Use timestamp delta as duration if possible
Created attachment 239608 [details] [review] Add support for MP4 timed text
First patch applied. Second patch: In gst_qt_mux_request_new_pad(), please use the AUDIO/VIDEO/SUBTITLE enum instead of name[0]. As I mentioned on IRC, I would like to see a test that generates a file that we can attempt to play in different players. But as we don't have that for other cases, it's not critical.
Also, I get this: CC libgstisomp4_la-atoms.lo atoms.c: In function 'atom_trak_set_subtitle_type': atoms.c:3480:25: error: variable 'entry' set but not used [-Werror=unused-but-set-variable]
I'll change it to enum, not sure how I could have missed the variable.
Created attachment 240183 [details] [review] Patch Fixed warning, left the other thing alone (as discussed on IRC)
Created attachment 240307 [details] [review] Patch Previous patch did not account for overlapping subtitles. There is no way to store overlapping subtitles in MP4. In order to not lose overlapped subtitles there needs to be some merging done in the muxer.
Review of attachment 239607 [details] [review]: Seems to be committed.
Review of attachment 240307 [details] [review]: Needs to be rebased.
Created attachment 254922 [details] [review] Patch to add support for timed text Rebased and slightly updated. I know it is a big patch, but would be nice if someone reviewed it. Been using it for a while and it seems to work well.
Review of attachment 254922 [details] [review]: ::: gst/isomp4/gstqtmux.c @@ +272,3 @@ } + if (params->subtitle_sink_caps) { Shouldn't you check for non-empty caps here? You use GST_STATIC_CAPS_NONE below @@ +575,3 @@ + + gst_buffer_map (buf, &map_old, GST_MAP_READ); + size = (gint16) _strnlen ((const char *) map_old.data, map_old.size); Why don't you just use strnlen() here? @@ +2521,3 @@ + GST_BUFFER_PTS (res) = time_span_min; + GST_BUFFER_DURATION (res) = time_span_max - time_span_min; + g_string_free (string, TRUE); You could just reuse the GString memory here and use gst_memory_new_wrapped()
> + if (params->subtitle_sink_caps) { > Shouldn't you check for non-empty caps here? You use GST_STATIC_CAPS_NONE below This is actually handled in gst_qt_mux_register params->subtitle_sink_caps = gst_static_caps_get (&prop->subtitle_sink_caps); if (gst_caps_is_empty (params->subtitle_sink_caps)) params->subtitle_sink_caps = NULL; > Why don't you just use strnlen() here? Not sure really, gstregistrychunks.c has its own implementation as well, I though that there are some compatibility issues > You could just reuse the GString memory here and use gst_memory_new_wrapped() Which flags should I use?
(In reply to comment #12) > > Why don't you just use strnlen() here? > Not sure really, gstregistrychunks.c has its own implementation as well, I > though that there are some compatibility issues I'm not aware of any :) > > You could just reuse the GString memory here and use gst_memory_new_wrapped() > Which flags should I use? None, i.e. 0 seems correct here
Looks like this has already been implemented (581295)
*** This bug has been marked as a duplicate of bug 581295 ***