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 752106 - flacparse: makes up bogus DTS for audio after seeking
flacparse: makes up bogus DTS for audio after seeking
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.2.4
Other Linux
: Normal blocker
: 1.5.91
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-07-08 07:36 UTC by sangkyu.choi
Modified: 2015-09-07 11:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Recalculate dts if dts exceeds pts (1.15 KB, patch)
2015-07-08 07:36 UTC, sangkyu.choi
rejected Details | Review
flacparse: set both pts and dts so baseparse doesn't make up wrong dts after a seek (2.73 KB, patch)
2015-09-04 18:47 UTC, Tim-Philipp Müller
accepted-commit_now Details | Review
wavpackparse: set both pts and dts so baseparse doesn't make up wrong dts after seeks (1.31 KB, patch)
2015-09-04 18:47 UTC, Tim-Philipp Müller
accepted-commit_now Details | Review

Description sangkyu.choi 2015-07-08 07:36:07 UTC
after seek(jump), some file pts faster than dts.
we need to adjust at least same.

Fix for playback after seek. Recalculate dts if dts exceeds pts.
Comment 1 sangkyu.choi 2015-07-08 07:36:45 UTC
Created attachment 307051 [details] [review]
Recalculate dts if dts exceeds pts
Comment 2 Tim-Philipp Müller 2015-07-08 07:53:16 UTC
I wonder if there isn't a bigger problem here.

Why do audio packets get their DTS set in the first place (by baseparse, presumably).

I don't think that should happen?
Comment 3 Sebastian Dröge (slomo) 2015-07-08 08:19:28 UTC
FLAC shouldn't have any DTS at all, or at least DTS should always be the same as PTS.
Comment 4 Sebastian Dröge (slomo) 2015-07-08 08:48:37 UTC
Is this a regression btw? Which GStreamer version are you testing?
Comment 5 Tim-Philipp Müller 2015-07-08 09:22:39 UTC
There is/was also an issue (possibly a bug report too) where baseparse puts dts on outgoing mp3 packets in mpegaudioparse even though tsdemux only ever puts pts on the input packets. I'm assuming this is all related, but it might not be of course.
Comment 6 Rajesh Singh 2015-07-08 09:59:37 UTC
Whenever a new buffer is arrived, its being parsed in the function “gst_flac_parse_parse_frame()” defined in file gstflacparse.c
This function is doing following things:
1.	It is calculating buffer PTS value every time based on sample number and sample rate, which is correct
    GST_BUFFER_TIMESTAMP (buffer) = gst_util_uint64_scale (flacparse->sample_number, flacparse->block_size * GST_SECOND, flacparse->samplerate);
2.	Here we are doing nothing to recalculate the DTS of the same buffer. DTS value is maintained by the base parse. 
3.	When that buffer is pushed by API “gst_pad_push()” , for certain streams, PTS is correct value but DTS is getting ahead of PTS. Which is an error. 
4.	This patch is an error check, under no conditions, DTS value should exceeds the PTS value.
5.	If this patch is not applied then for certain streams, we observe glitches/no sound after seek as, DTS value is getting exceeded by PTS.
Comment 7 Sebastian Dröge (slomo) 2015-07-08 10:29:35 UTC
So maybe flacparse should just unset DTS?
Comment 8 Rajesh Singh 2015-07-08 11:21:24 UTC
yes, the flacparser needs to assign DTS equal to PTS, for the first buffer after the seek.
For the normal contineous playback, the issue is not observed.
Comment 9 sangkyu.choi 2015-07-24 05:16:06 UTC
is there are any progress? ^^
Comment 10 Thiago Sousa Santos 2015-08-12 07:09:22 UTC
Can you provide some more details? What kind of files trigger this? FLAC? FLAC into some container? Does it always happen after a seek?
Comment 11 sangkyu.choi 2015-08-12 08:11:16 UTC
flac audio only file.
I downloaded it in commercial music streaming service.
After seek, the PTS value wrong for around 4 secs.

Dear Rajesh Singh, 

could you share a problem file?
Comment 12 Nicolas Dufresne (ndufresne) 2015-08-13 00:54:58 UTC
I also have the impression there is some bogus code in baseparse. Somewhere in the code, we have:

      if (next_pts >= parse->priv->next_dts)
        parse->priv->next_pts = next_pts;

But if it wasn't bigger, it left with an old (and probably worst) value.
Comment 13 Rajesh Singh 2015-08-13 05:27:34 UTC
Problematic/test stream file size is 118 MB,
Please share me, the efficient way to share the large problematic/test stream.
Comment 14 Thiago Sousa Santos 2015-08-13 08:53:09 UTC
You can put on your favorite online file sharing service/host and put the link here. Services that won't require login to download the file are preferred.
Comment 15 Sebastian Dröge (slomo) 2015-08-13 10:36:24 UTC
Nicolas, do you have suggestions what to change in the code to make it less weird?
Comment 16 Rajesh Singh 2015-08-13 11:56:57 UTC
Dear Thiago Sousa,
Can i divide the file in parts and send you over your email?
Comment 17 Thiago Sousa Santos 2015-08-13 13:28:32 UTC
Not ideal but if that is all you can do, please do it. Can I put the file somewhere public after receiving so other developers can check it, too?
Comment 18 Sebastian Dröge (slomo) 2015-08-15 09:50:34 UTC
Why is this a blocker? Is it a regression?

Arguably the audio parsers should never set DTS and only set PTS.
Comment 19 Rajesh Singh 2015-08-21 04:32:11 UTC
Dear Thiago Sousa,

I tried to send you, test stream over mail but mail delivery is getting failed
for mail id thiagossantos@gmail.com.

"Delivery failed: thiagossantos@gmail.com
Remote host said[Response Message]: 552-5.7.0 This message was blocked because its content presents a potential 552-5.7.0 security issue"
Comment 20 Tim-Philipp Müller 2015-09-04 14:06:40 UTC
Found a file I can reproduce this with, it yields this (between flacparse and flacdec):
...
dts: 0:00:05.146122448, pts:0:00:05.146122448
Seeking to 60.0 seconds..
dts: 0:01:00.104489796, pts:0:00:59.977142857
...
and the dts/pts diff remains for the rest.

This happens with 1.4 as well though.
Comment 21 Rajesh Singh 2015-09-04 17:03:24 UTC
Dear Tim,

Similar flac streams, I also found which I am unable to share due to huge size.
While doing seek near the end of duration, i observed difference between DTS and PTS even more than 5 seconds.

Above condition can be avoided, by assigning DTS equal to PTS.
This assignment of DTS equal to PTS, will be called only once after seek, if they 
differ.
Comment 22 Tim-Philipp Müller 2015-09-04 17:14:34 UTC
You're right indeed, the difference is larger towards the end:

Seeking to 200.0 seconds..
dts: 0:03:25.616326530, pts:0:03:19.993469387, duration: 0:00:00.026122449
Comment 23 Tim-Philipp Müller 2015-09-04 18:47:22 UTC
Created attachment 310678 [details] [review]
flacparse: set both pts and dts so baseparse doesn't make up wrong dts after a seek
Comment 24 Tim-Philipp Müller 2015-09-04 18:47:55 UTC
Created attachment 310679 [details] [review]
wavpackparse: set both pts and dts so baseparse doesn't make up wrong dts after seeks
Comment 25 Tim-Philipp Müller 2015-09-04 18:48:57 UTC
Comment on attachment 307051 [details] [review]
Recalculate dts if dts exceeds pts

This seems more like a workaround, better fix the actual problem. We know what the DTS should be here (either not set or same as PTS).
Comment 26 Sebastian Dröge (slomo) 2015-09-06 15:24:18 UTC
I assume you checked that none of the other audio parsers, and none of the video parsers are needed to be fixed the same way?
Comment 27 Tim-Philipp Müller 2015-09-06 15:42:23 UTC
It affects parsers for framed formats where timestamps are created based on information in the headers (like sample offset or frame offset). I've only fixed up audioparsers for now. Will have a look at the other GstBaseParse sub-classes later. In any case, it's not a regression anyway.commit fcdb79ef7b7ff55c8a64573cc991fca04dc7e590
Author: Tim-Philipp Müller <tim@centricular.com>
Date:   Fri Sep 4 19:45:37 2015 +0100

    wavpackparse: set both pts and dts so baseparse doesn't make up wrong dts after seeks
    
    https://bugzilla.gnome.org/show_bug.cgi?id=752106

commit 0d88f271084cce54de38907a618368deed843ee8
Author: Tim-Philipp Müller <tim@centricular.com>
Date:   Fri Sep 4 19:34:41 2015 +0100

    flacparse: set both pts and dts so baseparse doesn't make up wrong dts after a seek
    
    flac contains the sample offset in the frame header, so after a seek
    without index flacparse will know the exact position we landed on and
    timestamp buffers accordingly. It only set the pts though, which means
    the baseparse-set dts which was set to the seek position prevails, and
    since the seek was based on an estimate, there's likely a discrepancy
    between where we wanted to land and where we did land, so from here on
    that dts/pts difference will be maintained, with dts possibly multiple
    seconds ahead of pts, which is just wrong. The easiest way to fix this
    is to just set both pts and dts based on the sample offset, but perhaps
    parsed audio should just not have dts set at all.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=752106
Comment 28 Tim-Philipp Müller 2015-09-07 11:43:46 UTC
asfparse: probably could use some changes, but not even asfdemux or asfmux do any dts-handling yet, so might just as well leave it to when someone's interested in this.

opusparse: just makes up timestamps from 0 it seems (see bug #754669), probably not syncable anyway, so no issue there for this bug.

ivfparse: not syncable, so seeking will be based on index or scanning from good position, so 

videoparsers: vc1parse, h264parse, h265parse do some things, but they all lack explicit dts handling anyway and have well-known deficiencies in that area, so let's not touch them.