Created attachment 286762 [details] [review]
Patch

atom_trak_tx3g_update_dimension:
 - not sure why height is multiplied by 0.15; this doesn't seem right (although the dimensions are ignored by all players I tested this with)

atom_trak_set_subtitle_commons:
 - subtitle track should have alternate group set; for now set it to 2 for all subtitles (all other track have zero). 
 - layer should be above video, which has layer 0.

gst_qt_mux_prepare_tx3g_buffer: 
- tx3g buffer should not be null terminated. use strnlen to get actual length
 - buffer duration must be set manually. gst_buffer_copy_into doesn't copy duration when length differs (seriously though, that's not very intuitive)

gst_qt_mux_add_buffer:
 - should not compute DTS for sparse streams, as the gaps are handled later using empty buffer

gst_qt_mux_request_new_pad:
 - gst_collect_pads_add_pad should be called with lock = FALSE for sparse streams, otherwise the pipeline stalls waiting for subtitle buffer that might not come

Do you have a launch line and/or a sample that could be use to test and reproduce the issues?

I don't. This is with custom pipeline. Hover it shouldn't matter how you mux it, the sample duration will still be 0 (gst_buffer_copy_into doesn't copy duration) and PTS will still be off (because of computed DTS), so the problems shouldn't be difficult to reproduce.

Review of attachment 286762 [details] [review]:

I would just leave the 0.15 thing as it seems to be the recommended value from the spec.

For the other changes I'd prefer having smaller patches where we could put an specific commit message for each. Makes it easier to review and track changes.

So, I'd propose 1 patch for the lock at collect pads, 1 for the duration and buffer length fixes and another one for the atoms.c changes. What do you think?

::: gst/isomp4/atoms.c
@@ -2939,3 @@
     tx3g->font_size = 0.05 * height;
 
-    height = 0.15 * height;

"The subtitle track’s height should be 0.15 * the 'vide' track header height. This allows room for two lines
of subtitle text. For example, if the 'vide' track header height is 720 pixels, then the 'sbtl ' track header
height should be 108 (pixels)."

According to qtff spec.

@@ +3428,3 @@
+
+  trak->tkhd.alternate_group = 2;       /* same for all subtitles */
+  trak->tkhd.layer = -1;        /* above video (layer 0) */

Could you put this in a patch separate? It seems unrelated to the other changes and it would be easier to track regressions or understand the commit better.

My apologies, I must have missed the part in specs. Makes sense since it's the text bounds. All the "real world" files that I have seen had usually just some "hardcoded" values that wasn't even related to the video. No player that I have tested respects those and other formatting values. Not even if style box is inserted after text cue, which according to specs should work. I an imagine one of the reason would be that pretty everyone muxes the files with just some made up values so the playback results would be pretty inconsistent.

I will split the patch tomorrow and resubmit. Thanks for looking into this.

Created attachment 286898 [details] [review]
Patch to move subtitle layer above video

Created attachment 286899 [details] [review]
Patch to make subtitle buffer not null terminated and set proper duration

Created attachment 286900 [details] [review]
Patch to not compute DTS on subtitle buffer

Created attachment 286901 [details] [review]
Patch to create collects pad with lock=false for subtitle track

Pushed 3 of your 4 patches.

commit fd3e8c5672afe9245144193aae0470496afe9546
Author: Matej Knopp <matej.knopp@gmail.com>
Date:   Tue Sep 23 19:08:48 2014 +0200

    qtmux: collect pad for sparse stream should be created with lock set to false
    
    Avoids waiting for buffers from sparse streams
    
    https://bugzilla.gnome.org/show_bug.cgi?id=737095

commit 669534158329fd69150987d1f8a6d3979b231057
Author: Matej Knopp <matej.knopp@gmail.com>
Date:   Tue Sep 23 19:07:25 2014 +0200

    qtmux: fix subtitle buffer duration and strip null termination
    
    Strip the \0 off the subtitle as we already know the size and also remember
    to set the duration as buffer copying doesn't do it.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=737095

commit f57e9c4516b66d330165df9f4d6c82dc12d00be4
Author: Matej Knopp <matej.knopp@gmail.com>
Date:   Tue Sep 23 19:06:18 2014 +0200

    qtmux: move subtitle layer above video and set alternate group
    
    layer -1 is above video, that is 0
    And having all subtitles in alternate group 2 means that only one
    should be selected at a time.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=737095

Comment on attachment 286899 [details] [review]
Patch to make subtitle buffer not null terminated and set proper duration

Reworked the commit message a bit and pushed

Comment on attachment 286900 [details] [review]
Patch to not compute DTS on subtitle buffer

Can you elaborate more on the commit message? This change is a bit hard to understand directly from the code. At least the consequences are.

Also, please use qtmux: instead of gstqtmux as we usually go for the element name rather than the file name.

Not sure what the message would be. Does something along

qtmux: Do not infer buffer DTS for sparse streams

make more sense?

(In reply to comment #15)
> Not sure what the message would be. Does something along
> 
> qtmux: Do not infer buffer DTS for sparse streams
> 
> make more sense?

It would be good to give some context on how qtmux calculates the durations. Previous buffer ts - next one ts, and why it  doesn't make sense for sparse streams.

as a commit message? shouldn't that perhaps be in code instead?

(In reply to comment #17)
> as a commit message? shouldn't that perhaps be in code instead?

It can be put in the code as well.

The commit log also helps future developers to have more context on changes in case they want to understand what caused some lines to change. git blame is very helpful for this.

Created attachment 286938 [details] [review]
Patch with more descriptive commit message improved comment

commit 9f85dfd733792dc5a49acd32f657c326812dc9af
Author: Matej Knopp <matej.knopp@gmail.com>
Date:   Tue Sep 23 23:33:37 2014 +0200

    qtmux: Do not infer DTS on buffers from sparse streams.
    
    DTS delta is used to calculate sample duration. If buffer has missing DTS, we take either segment start or previous buffer end time, whichever is later.
    This must only be done for non sparse streams, sparse streams can have gaps between buffers (which is handled later by adding extra empty buffer with duration that fills the gap)
    
    https://bugzilla.gnome.org/show_bug.cgi?id=737095