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 774840 - qtmux: Fix various timestamp and duration related issues
qtmux: Fix various timestamp and duration related issues
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other All
: Normal normal
: 1.11.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-11-22 14:23 UTC by Sebastian Dröge (slomo)
Modified: 2017-02-23 11:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
qtmux: Only write "gap" edit list if there is a non-zero gap (1.40 KB, patch)
2016-11-22 14:23 UTC, Sebastian Dröge (slomo)
committed Details | Review
qtmux: Revert commits that set DTS and duration on buffers unconditionally (2.75 KB, patch)
2016-11-22 14:23 UTC, Sebastian Dröge (slomo)
committed Details | Review
qtmux: Only use buffer durations if they are actually valid (4.20 KB, patch)
2016-11-22 14:24 UTC, Sebastian Dröge (slomo)
committed Details | Review
qtmux: Error out much earlier if we don't have a valid PTS (1.25 KB, patch)
2016-11-22 14:43 UTC, Sebastian Dröge (slomo)
committed Details | Review
qtmux: Make sure to only change DTS of writable buffers (1.38 KB, patch)
2016-11-22 14:43 UTC, Sebastian Dröge (slomo)
committed Details | Review

Description Sebastian Dröge (slomo) 2016-11-22 14:23:48 UTC
See commit messages
Comment 1 Sebastian Dröge (slomo) 2016-11-22 14:23:52 UTC
Created attachment 340517 [details] [review]
qtmux: Only write "gap" edit list if there is a non-zero gap
Comment 2 Sebastian Dröge (slomo) 2016-11-22 14:23:58 UTC
Created attachment 340518 [details] [review]
qtmux: Revert commits that set DTS and duration on buffers unconditionally

39f7e52266fde3b3c035e22cbcbb2bb1fa207b17 was setting the buffer duration
to 0 if is not valid, under the assumption that this is "the last"
buffer and no others are coming next. This is wrong, last_buf is the
previous buffer and not the very last one.

4e3c13c87c258c9c95e2217d32ab314d12b5fffc was setting DTS to 0 if there
was none. This will set DTS to 0 for all e.g. audio streams, completely
messing up calculations if streams don't start at 0.
Comment 3 Sebastian Dröge (slomo) 2016-11-22 14:24:03 UTC
Created attachment 340519 [details] [review]
qtmux: Only use buffer durations if they are actually valid
Comment 4 Sebastian Dröge (slomo) 2016-11-22 14:43:43 UTC
Created attachment 340520 [details] [review]
qtmux: Error out much earlier if we don't have a valid PTS
Comment 5 Sebastian Dröge (slomo) 2016-11-22 14:43:48 UTC
Created attachment 340521 [details] [review]
qtmux: Make sure to only change DTS of writable buffers

And trivial cleanup
Comment 6 Sebastian Dröge (slomo) 2016-11-23 16:17:18 UTC
Attachment 340517 [details] pushed as 9acc0b5 - qtmux: Only write "gap" edit list if there is a non-zero gap
Attachment 340518 [details] pushed as 351f2fc - qtmux: Revert commits that set DTS and duration on buffers unconditionally
Attachment 340519 [details] pushed as 6fb5f85 - qtmux: Only use buffer durations if they are actually valid
Attachment 340520 [details] pushed as 01e45b2 - qtmux: Error out much earlier if we don't have a valid PTS
Attachment 340521 [details] pushed as 01682ae - qtmux: Make sure to only change DTS of writable buffers
Comment 7 Andreas Frisch 2017-02-23 11:40:23 UTC
I'm having issues with this.
I've git bisected a problem that causes audio to sound skippy and broken on a pipeline that looks like this:

gst-launch-1.0 filesrc location=testsrc-av.mkv ! matroskademux name=d  d. ! aacparse ! queue ! m. qtmux name=m streamable=true faststart=true ! filesink location=audio-bisect.mp4

with the source file generated this way:
gst-launch-1.0 videotestsrc pattern=ball is-live=1 ! timeoverlay ! video/x-raw,framerate=30/1,width=1280,height=720 ! videoconvert ! queue max-size-buffers=1 ! x264enc speed-preset=superfast tune=zerolatency key-int-max=1 b-adapt=0 option-string=scenecut=0 ! video/x-h264,profile=main ! m. audiotestsrc is-live=1 ! audioconvert ! faac ! queue ! m. matroskamux name=m streamable=true timecodescale=1000 ! filesink location=testsrc-av.mkv

i've git bisected this (it still sounded okay in 1.10.3) and it came down to this:

51f2fca0a39cad58436ab83ae76ea6439de11e8 is the first bad commit
commit 351f2fca0a39cad58436ab83ae76ea6439de11e8
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Tue Nov 22 15:59:19 2016 +0200

    qtmux: Revert commits that set DTS and duration on buffers unconditionally
    
    39f7e52266fde3b3c035e22cbcbb2bb1fa207b17 was setting the buffer duration
    to 0 if is not valid, under the assumption that this is "the last"
    buffer and no others are coming next. This is wrong, last_buf is the
    previous buffer and not the very last one.
    
    4e3c13c87c258c9c95e2217d32ab314d12b5fffc was setting DTS to 0 if there
    was none. This will set DTS to 0 for all e.g. audio streams, completely
    messing up calculations if streams don't start at 0.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=774840

:040000 040000 27866870bd6bd3151132fb258591cbb34787c790 4b51c225f3684daccd2ca996052ff0b2a9834ab0 M      gst