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 737095 - qtmux: subtitle muxing doesn't work
qtmux: subtitle muxing doesn't work
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other All
: Normal normal
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-09-21 22:14 UTC by Matej Knopp
Modified: 2014-09-24 01:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch (3.95 KB, patch)
2014-09-21 22:16 UTC, Matej Knopp
reviewed Details | Review
Patch to move subtitle layer above video (797 bytes, patch)
2014-09-23 17:10 UTC, Matej Knopp
committed Details | Review
Patch to make subtitle buffer not null terminated and set proper duration (1.60 KB, patch)
2014-09-23 17:10 UTC, Matej Knopp
committed Details | Review
Patch to not compute DTS on subtitle buffer (1.13 KB, patch)
2014-09-23 17:10 UTC, Matej Knopp
reviewed Details | Review
Patch to create collects pad with lock=false for subtitle track (1.38 KB, patch)
2014-09-23 17:11 UTC, Matej Knopp
committed Details | Review
Patch with more descriptive commit message improved comment (1.66 KB, patch)
2014-09-23 21:37 UTC, Matej Knopp
committed Details | Review

Description Matej Knopp 2014-09-21 22:14:00 UTC
Current tx3d muxing implementation has some issues.
Comment 1 Matej Knopp 2014-09-21 22:16:28 UTC
Created attachment 286762 [details] [review]
Patch
Comment 2 Matej Knopp 2014-09-21 22:25:24 UTC
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
Comment 3 Thiago Sousa Santos 2014-09-22 14:57:25 UTC
Do you have a launch line and/or a sample that could be use to test and reproduce the issues?
Comment 4 Matej Knopp 2014-09-22 15:13:35 UTC
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.
Comment 5 Thiago Sousa Santos 2014-09-22 20:08:57 UTC
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.
Comment 6 Matej Knopp 2014-09-22 20:53:40 UTC
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.
Comment 7 Matej Knopp 2014-09-22 20:54:02 UTC
I will split the patch tomorrow and resubmit. Thanks for looking into this.
Comment 8 Matej Knopp 2014-09-23 17:10:03 UTC
Created attachment 286898 [details] [review]
Patch to move subtitle layer above video
Comment 9 Matej Knopp 2014-09-23 17:10:32 UTC
Created attachment 286899 [details] [review]
Patch to make subtitle buffer not null terminated and set proper duration
Comment 10 Matej Knopp 2014-09-23 17:10:57 UTC
Created attachment 286900 [details] [review]
Patch to not compute DTS on subtitle buffer
Comment 11 Matej Knopp 2014-09-23 17:11:25 UTC
Created attachment 286901 [details] [review]
Patch to create collects pad with lock=false for subtitle track
Comment 12 Thiago Sousa Santos 2014-09-23 18:30:16 UTC
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 13 Thiago Sousa Santos 2014-09-23 18:32:14 UTC
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 14 Thiago Sousa Santos 2014-09-23 18:33:09 UTC
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.
Comment 15 Matej Knopp 2014-09-23 19:24:51 UTC
Not sure what the message would be. Does something along

qtmux: Do not infer buffer DTS for sparse streams

make more sense?
Comment 16 Thiago Sousa Santos 2014-09-23 20:14:28 UTC
(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.
Comment 17 Matej Knopp 2014-09-23 20:31:19 UTC
as a commit message? shouldn't that perhaps be in code instead?
Comment 18 Thiago Sousa Santos 2014-09-23 20:44:40 UTC
(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.
Comment 19 Matej Knopp 2014-09-23 21:37:02 UTC
Created attachment 286938 [details] [review]
Patch with more descriptive commit message improved comment
Comment 20 Thiago Sousa Santos 2014-09-24 01:32:58 UTC
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